Open Dollar - m4k2's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/ODSafeManager.sol#L49-L53 https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/ODSafeManager.sol#L105-L109 https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/proxies/ODSafeManager.sol#L155-L232 https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/proxies/ODSafeManager.sol#L241-L252

Vulnerability details

The modifier safeAllowed() in ODSafeManager doesn't work as intended and will always revert making the user unable to interact with its own safe

Impact

• HIGH : the protocol is not meant to work like this, user can't reach functions related to their tokens.

The modifier safeAllowed() in ODSafeManager is used as an Access Control mean.

Given a Safe ID, it verifies one of the two following conditions :

• the msg.sender is the owner of the safe
• the msg.sender is allowed to interact with this safe (using the safeCan state variable)

In the protocol, the owner of a safe is supposed to be an ODProxy (owned by a user) and users interact with the protocol through their own ODProxy.

This means that every ODSafeManager functions protected by the safeAllowed() modifier can be executed by the allowed ODProxy (unless another address has been allowed to interact with it with safeCan(address,uint256,address) BUT this has to be done using the allowSAFE() function which is also protected by the safeAllowed() modifier, making it unreachable).

Functions unreachable due to this bug : allowSAFE(), modifySAFECollateralization(), transferCollateral(), transferInternalCoins(), enterSystem(), quitSystem(), moveSAFE(), removeSAFE(), protectSAFE().

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

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

Taking example on the allowSAFE function :

modifier safeAllowed(uint256 _safe) {
    address _owner = _safeData[_safe].owner;
    if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed();
    _;
}

...

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);
}

The issue here is that the ODProxy interacts with other contracts (such as ODSafeManager) using delegatecall

https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/ODProxy.sol#L26-L35

function execute(address _target, bytes memory _data) external payable onlyOwner returns (bytes memory _response) {
    if (_target == address(0)) revert TargetAddressRequired();

    bool _succeeded;
    (_succeeded, _response) = _target.delegatecall(_data);

    if (!_succeeded) {
        revert TargetCallFailed(_response);
    }
}

A delegatecall keeps the msg.sender value of the initial caller (the initial EOA address that tries to interact with the protocol) thus, the modifier will always revert as the safe owner will always be different than the msg.sender

Proof of Concept

This file should be located in test/BrokenModifier.t.sol and can be executed with

forge test --match-contract BrokenModifier --match-test testBrokenModifier -vvv

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

import {Test, console} from "forge-std/Test.sol";
import {Vault721} from "src/contracts/proxies/Vault721.sol";
import {ODProxy} from '@contracts/proxies/ODProxy.sol';
import {BasicActions} from '@contracts/proxies/actions/BasicActions.sol';
import {ODSafeManager} from '@contracts/proxies/ODSafeManager.sol';
import {SAFEEngine} from '@contracts/SAFEEngine.sol';
import '@interfaces/ISAFEEngine.sol';

contract BrokenModifier is Test {
    address public deployer = address(1);
    address public friend = address(2);
    address public governor = address(3);
    address public legitUser = address(4);
    Vault721 public vault721;
    ODSafeManager public safeManager;
    SAFEEngine public safeEngine;
    BasicActions public basicActions;
    ISAFEEngine.SAFEEngineParams public safeEngineParameters = ISAFEEngine.SAFEEngineParams(
        10 ** 18, // WAD
        10 ** 45 // RAD
    );

    function setUp() public {
        vm.startPrank(deployer);
        vault721 = new Vault721(governor);
        safeEngine = new SAFEEngine(safeEngineParameters);
        safeManager = new ODSafeManager(address(safeEngine), address(vault721));
        basicActions = new BasicActions();
        vm.stopPrank();
    }

    function testBrokenModifier() public {
        // first we build a proxy for a user
        vm.startPrank(legitUser);
        vault721.build(legitUser);
        ODProxy userProxy = ODProxy(vault721.getProxy(legitUser));
        bytes memory payload = abi.encodeWithSelector(
            basicActions.openSAFE.selector, address(safeManager), "", address(userProxy)
        );
        bytes memory safeData = userProxy.execute(
            address(basicActions),
            payload
        );

        uint256 userSafeId = abi.decode(safeData, (uint256));
        // userProxy owns the SAFE : the state of the ODSafeManager has been updated
        assertEq(
            safeManager.safeData(userSafeId).owner,
            address(userProxy)
        );

        // here we try to allow the address of our "friend" by setting the value 1 in the
        // `safeCan` mapping on ODSafeManager
        payload = abi.encodeWithSelector(
            safeManager.allowSAFE.selector, userSafeId, friend, 1
        );

        // this will revert in the modifier because msg.sender and _owner are always different
        // because of the delegatecall
        userProxy.execute(
            address(safeManager),
            payload
        );
    }

}

In the output of the command, we can clearly see that the transaction revert with SafeNotAllowed() in the first place

    ...
    ├─ [970] ODSafeManager::safeData(1) [staticcall]
    │   └─ ← (0x582e4a5B8F0c154922e086061cC6Dd07F056EAC1, 0x4794a3F9d446e0D658ceaa048a26d58aa9A9725E, 0x0000000000000000000000000000000000000000000000000000000000000060)
    ├─ [6698] ODProxy::execute(ODSafeManager: [0xc051134F56d56160E8c8ed9bB3c439c78AB27cCc], 0xe0decbcd000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000001)
    │   ├─ [5041] ODSafeManager::allowSAFE(1, 0x0000000000000000000000000000000000000002, 1) [delegatecall]
    │   │   └─ ← "SafeNotAllowed()"
    │   └─ ← "TargetCallFailed(0xdba06d65)"
    └─ ← "TargetCallFailed(0xdba06d65)"

Test result: FAILED. 0 passed; 1 failed; finished in 3.91ms

Failing tests:
Encountered 1 failing test in test/exploits/BrokenModifier.t.sol:BrokenModifier
[FAIL. Reason: TargetCallFailed(0xdba06d65)] testBrokenModifier() (gas: 748309)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools used

Foundry

Recommended Mitigation Steps

We recommend one of the following mitigation steps depending on how the protocol should behave :

• Update the modifier to allow the owner of proxy to execute the functions
• Add a new function in ODProxy to call (and not delegatecall) on the target contract

Assessed type

Access Control

Lack of ACCES CONTROL on addSAFE(uint256 _safe) in ODSafeManager letting anyone call this function with arbitrary value

Impact

LOW as it doesn't really impact the protocol

addSAFE function

// @audit arbitrary call to add a safe to yourself ? 
  function addSAFE(uint256 _safe) external {
    SAFEData memory _sData = _safeData[_safe];
    _usrSafes[msg.sender].add(_safe);
    _usrSafesPerCollat[msg.sender][_sData.collateralType].add(_safe);
  }

  /// @inheritdoc IODSafeManager
  function removeSAFE(uint256 _safe) external safeAllowed(_safe) {
    SAFEData memory _sData = _safeData[_safe];
    _usrSafes[_sData.owner].remove(_safe);
    _usrSafesPerCollat[_sData.owner][_sData.collateralType].remove(_safe);
  }

addSAFE(uint256 _safe) doesn't have the safeAllowed(_safe) modifier as removeSAFE(uint256 _safe) have.

In this case Anyone can add an arbitrary safe to his _usrSafes[] list.

Recommended Mitigation Steps

Add the modifier safeAllowed(_safe) to the addSAFE(uint256 _safe).