Open Dollar - merlin's results

Lines of code

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

Vulnerability details

Bug Description

In the ODSafeManager smart contract, there exists an allowSAFE function that permits or restricts a user's ability to manage the safe. If the owner of the SAFE attempts to revoke access from a user with permission, there is a potential vulnerability where the allowed user could front-run the transaction to add another user. Subsequently, after the removal operation is executed, the initially removed user can leverage the newly added user to regain their authority.

Proof-of-Concept

The following test case illustrates the scenario:

Alice grants permission for Bob to manage the SAFE using the function: allowSAFE(1, Bob, 1). Subsequently, Bob's intentions turn malicious, and Alice decides to revoke his access. As Alice initiates a transaction to disallow Bob, he detects this and front-runs her tx by authorizing a new user with permission with the function: allowSAFE(1, Erin, 1). When Alice's transaction is executed, it successfully disallows Bob using the function: allowSAFE(1, Bob, 0). Now, Erin authorizes Bob to manage the SAFE once again with the function: allowSAFE(1, Bob, 1).

function allowSAFE(
        uint256 _safe,
        address _usr,
        uint256 _ok
    ) external safeAllowed(_safe) {
        address _owner = _safeData[_safe].owner;
        safeCan[_owner][_safe][_usr] = _ok;
        emit AllowSAFE(msg.sender, _safe, _usr, _ok);
    }

Impact

This would allow the malicious user with permission to keep executing txs on behalf of the owner of the SAFE.

Tools Used

Manual review

Recommended Mitigation Steps

Prevent users who have permission from invoking the allowSAFE() function on behalf of the SAFE owner:

function allowSAFE(
        uint256 _safe,
        address _usr,
        uint256 _ok
-    ) external safeAllowed(_safe) {
+    ) external {
				 address _owner = _safeData[_safe].owner;
+       if (msg.sender != _owner)
+           revert HandlerNotAllowed();
        safeCan[_owner][_safe][_usr] = _ok;
        emit AllowSAFE(msg.sender, _safe, _usr, _ok);
    }

Assessed type

Access Control