Approved address can approve other addresses for an owner's safe

mediumCode4rena

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L105

Vulnerability details

Impact

An owner of a safe can give permissions/approval of their safe to another address (let's say address B) through the allowSafe() function in the ODSafeManager.sol contract. But this other address (address B) also gets the power to approve other addresses for the owner's safe. This is a permissioning problem in the allowSafe() function (specifically the safeAllowed() modifier) which creates a security risk for the owner's safe.

This might look like a design choice initially but it has been confirmed as an issue with the sponsor:

Proof of Concept

Here is the whole process:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L105C1-L109C4

1. Owner of the safe gives permission/approval to address _usr (address 0x01 for example purposes) for uint256 _safe through the allowSafe() function.

• On Line 107, safeCan[_owner][_safe][_usr] = _ok; is set to any value other than 0 to represent approval.

solidity📋
File: ODSafeManager.sol
105:   function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {
106:     address _owner = _safeData[_safe].owner;
107:     safeCan[_owner][_safe][_usr] = _ok;
108:     emit AllowSAFE(msg.sender, _safe, _usr, _ok);
109:   }

2. The previously set address _usr (address 0x01) can now call the allowSafe() function with parameters uint256 _safe which will be the owner's safe and another address _usr (address 0x02), which will give address 0x02 permissions/approval for the owner's safe.

3. This issue arises because of how the checks are evaluated in the safeAllowed() modifier. Here is what happens:

• On Line 50, the owner of the safe is extracted.
• On Line 51, there are two conditions present that are separated by the && operator.
• On Line 51, the first check evaluates to true, since the msg.sender (address 0x01) is not the owner of the safe
• On Line 51, the second check evaluates to false, since the msg.sender (address 0x01) was previously approved by the owner in step 1 above.
• Since true && false = false, we do not revert and this gives address 0x02 permissions to the owner's safe in the allowSafe() function.

solidity📋
File: ODSafeManager.sol
49:   modifier safeAllowed(uint256 _safe) {
50:     address _owner = _safeData[_safe].owner;
51:     if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed();
52:     _;
53:   } 

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing a separate modifier for the allowSafe() function that only checks if the msg.sender is the owner. If true, then allow execution but if not then revert.

Solution:

solidity📋
File: ODSafeManager.sol
modifier onlySafeOwner(uint256 _safe) {
    address _owner = _safeData[_safe].owner;
    if (msg.sender != _owner) revert SafeNotAllowed();
    _;
}

Assessed type

Access Control