Open Dollar - ge6a's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODProxy.sol#L26-L35 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L1-L254 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/actions/BasicActions.sol#L1-L401

Vulnerability details

Description

One of the main invariants of the protocol is that users must exclusively use the ODProxy to interact with their safes. After analysis of the protocol, it can be seen that this can only happen if the proxy owns the safe, not the user themselves. The ODSafeManager contract contains the primary functions for working with safes. Most functions in it are protected by the 'safeAllowed' modifier, which checks whether 'msg.sender' is the owner of the safe or has been granted the right to work with it. Since the proxy contract uses 'delegatecall,' ODSafeManager cannot be called directly due to the change in context. Interaction with ODSafeManager occurs through the BasicActions contract, which is invoked by ODProxy. The problem is that BasicActions lacks some essential functions from ODSafeManager: 'allowSafe,' 'allowHandler,' 'quitSystem,' 'enterSystem,' 'moveSafe,' 'addSafe,' 'removeSave,' and 'protectSafe.' From what has been described so far, it can be concluded that the user cannot execute any of them.

Impact

It is clear that core functionalities of the protocol are broken, and users cannot use them. I don't want to go into details about most of the functions above because I am not familiar with how safes work, which is beyond the scope of this audit. But I will focus on the 'allowSafe' function. It allows the transfer of control of the safe to another user, similar to 'approve' in ERC20. The lack of this function can lead to indirect financial losses for users – for example, delayed responses to certain events due to the inability for more than one user to have control over the safe. Furthermore, a user may be under the wrong impression that such a function is available.

Proof of Concept

Here are 2 tests that show how both ways to access ODSafeManager.allowSAFE don't work (i put them into test/nft/anvil/NFT.t.sol). It is similar for the other functions.

  function test_allowSAFE_direct() public
  {
      address proxy = proxies[0];
      bytes32 cType = cTypes[0];
      uint256 vaultId = vaultIds[proxy][cType];

      vm.startPrank(users[0]);
      safeManager.allowSAFE(vaultId, proxies[1], 1000);
      bytes4 selector = bytes4(keccak256("SafeNotAllowed()"));
      vm.expectRevert(selector);
      vm.stopPrank();
  }

  function test_allowSAFE_proxy() public
  {
      address proxy = proxies[0];
      bytes32 cType = cTypes[0];
      uint256 vaultId = vaultIds[proxy][cType];

      vm.startPrank(users[0]);
      bytes memory payload = abi.encodeWithSelector(safeManager.allowSAFE.selector, address(safeManager), vaultId, proxies[1], 1000);
      bytes memory safeData = ODProxy(proxy).execute(address(safeManager), payload);
      vm.expectRevert();
      vm.stopPrank();
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Add wrappers in BasicActions for the functions described above so that users could execute them through ODProxy.

Assessed type

call/delegatecall