Lines of code

Vulnerability details

Impact

Member roles in SecurityCouncilManager contract can change owners of GnosisSafe(securityCouncil) contracts by schedulinig a perform call to ArbitrumTimelock contract.

However the contract that handles updating owners(security council members) with perform function is SecurityCouncilMemberSyncAction contract, But perform function has no validation to see whether the caller(msg.sender) is allowed to update security council members or not

This means anyone have access to changing the security council members in GnosisSafe contracts and they can make themselves security council members and access any setter or any other function that requires caller to be GnosisSafe contract, this can cause loss of funds and break most of the operations or even distroy the whole protocol based on the functions that attacker will get access to.

perform function after validating the passed nonce and checking to see if new members already exist, it calls _addMember and _removeMember internal functions, they also doesn’t have any validation for caller and they call _execFromModule function to call GnosisSafe contract which set new security council members and remove all previous members

Proof of Concept

This is perform function which doesn’t validiate the caller (I removed the comments)

31:     function perform(address _securityCouncil, address[] memory _updatedMembers, uint256 _nonce)
32:         external
33:         returns (bool res)
34:     {
...
42:         uint256 updateNonce = getUpdateNonce(_securityCouncil);
43:         if (_nonce <= updateNonce) {
...
46:             emit UpdateNonceTooLow(_securityCouncil, updateNonce, _nonce);
47:             return false;
48:         }
49: 
...
52:         _setUpdateNonce(_securityCouncil, _nonce);
53: 
54:         IGnosisSafe securityCouncil = IGnosisSafe(_securityCouncil);
...
56:         uint256 threshold = securityCouncil.getThreshold();
57: 
58:         address[] memory previousOwners = securityCouncil.getOwners();
59: 
60:         for (uint256 i = 0; i < _updatedMembers.length; i++) {
61:             address member = _updatedMembers[i];
62:             if (!securityCouncil.isOwner(member)) {
63:                 _addMember(securityCouncil, member, threshold);
64:             }
65:         }
66: 
67:         for (uint256 i = 0; i < previousOwners.length; i++) {
68:             address owner = previousOwners[i];
69:             if (!SecurityCouncilMgmtUtils.isInArray(owner, _updatedMembers)) {
70:                 _removeMember(securityCouncil, owner, threshold);
71:             }
72:         }
73:         return true;
74:     }

And this is _execFromModule function:

127:     function _execFromModule(IGnosisSafe securityCouncil, bytes memory data) internal {
128:         if (
129:             !securityCouncil.execTransactionFromModule(
130:                 address(securityCouncil), 0, data, OpEnum.Operation.Call
131:             )
132:         ) {
133:             revert ExecFromModuleError({data: data, securityCouncil: address(securityCouncil)});
134:         }
135:     }

Tools Used

Manual Review

Recommended Mitigation Steps

Since you schedule perform call to ArbitrumTimelock contract, you should only allow ArbitrumTimelock to call the perform function

Assessed type

Access Control

---

The text was updated successfully, but these errors were encountered:

All reactions