Open Dollar - Greed's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/Vault721.sol#L85-L88 https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/actions/BasicActions.sol#L226-L228 https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/ODSafeManager.sol#L105-L109

Vulnerability details

Impact

• Medium : the protocol is not meant to work like this

A user is meant to interact with the protocol through the use of its personal ODProxy which will execute different actions on the ODSafeManager.

In the following scenario, User A wants to create a new safe and allow his friend, User B to access the same safe.

https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/Vault721.sol#L85-L88

https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/actions/BasicActions.sol#L226-L228

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

Based upon how the protocol is supposed to work, in order to do that a user would :

• create its ODProxy using the build() function in Vault721.sol (this will work as intended)
• create a new SAFE by telling its ODProxy to execute() the openSAFE() function on BasicActions.sol which is going to call the openSAFE() function in ODSafeManager (this will work as intended)
• allow its friend to access the safe by telling its ODProxy to execute() the allowSAFE() function on ODSafeManager.sol directly (this will fail everytime)

The problem here comes from the fact that ODProxy uses a delegatecall() to execute the given functions on a target contract

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

Thus, these functions will be executed in the context of the ODProxy including its state and storage variables BUT these ODProxy don't have any state variable other than the variable OWNER

To summerize, as soon as an ODProxy tries to interact directly with another contract in order to change this contract's state (such as ODSafeManager.sol) this will fail because of the delegatecall

Proof of concept

In this PoC, we are going to do the exact same thing that was explained previously in order to show that the proxy can't interact with the ODSafeManager

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

forge test --match-contract RevertManager --match-test testRevertManager

// 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 RevertManager 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 testRevertManager() 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 as the proxy tries to interact directly with the ODSafeManager
        // but using the proxy's context/state (which is empty)
        vm.expectRevert();
        userProxy.execute(
            address(safeManager),
            payload
        );

        // we can see that the mapping has not been changed and is still 0
        assertEq(
            safeManager.safeCan(legitUser, userSafeId, friend),
            0
        );
    }

}

Tools used

Foundry

Recommended Mitigation Steps

• Add a new function in ODProxy to call (and not delegatecall) on the target contract

Assessed type

call/delegatecall