Open Dollar - 0xlemon's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/ODProxy.sol#L30 https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/ODSafeManager.sol#L49-L52 https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/Vault721.sol#L190-L197 https://github.com/open-dollar/od-contracts/blob/dev/src/contracts/proxies/ODSafeManager.sol#L136-L152

Vulnerability details

Summary

By using delegatecall in the ODProxy.sol contract we break the logic of the ODSafeManager.sol contract because it expects the msg.sender to be the proxy in all the calls, however that is not the case since delegatecall keeps the sender as the original one.

Vulnerability Details

ODSafeManager.sol contract uses a mapping _safeData between the safeId and the SAFEData structure(where the owner of that safe is stored). The address of the owner of this safe is the address of the proxy which was previously deployed. The issue arises when we do a delegatecall through the ODProxy.sol contract because we have a modifier attached to specific functions(safeAllowed) that checks if the msg.sender is the owner of the proxy or if the msg.sender was granted a permission to execute these functions. However the msg.sender will not be the proxy's address, it will be the user's, because delegatecall preserves the msg.sender and msg.value which means that this modifier will always revert because the proxy is never the msg.sender. It was confirmed by the sponsors that users are supposed to call the ODSafeManager.sol contract through their proxies however this leads to users not being able to execute any function with the safeAllowed modifier. Another thing is that when we do a delegatecall this means that this call executes in the context of ODProxy.sol, meaning any changes to the state of ODSafeManager.sol will not be made.

Impact

Users not being able to interact with the ODSafeManager.sol as expected.

Proof of Concept

Here is a test that proves my point. You need to add this function in the NFTAnvil.sol contract and run it using: forge t --fork-url http://127.0.0.1:8545 --match-contract NFTAnvil --match-test test_IncorrectUseOfDelegateCall -vvvv.

function test_IncorrectUseOfDelegateCall(uint256 vaultId) public {
  vaultId = bound(vaultId, 1, totalVaults - 1);
  address owner = vault721.ownerOf(vaultId);

  address receiver = makeAddr("receiver");

  vm.startPrank(owner);
  vault721.transferFrom(owner, receiver, vaultId);
  vm.stopPrank();

  IODSafeManager.SAFEData memory sData = safeManager.safeData(vaultId);

  address createdProxyAddress = sData.owner;

  address newOwner = vault721.ownerOf(vaultId);

  vm.expectRevert();
  vm.startPrank(receiver);
  ODProxy(createdProxyAddress).execute(address(safeManager), 
  abi.encodeWithSelector(safeManager.allowSAFE.selector, vaultId, owner, 1));
  vm.stopPrank();
}

We can see that after we transfer a vault to the user, he becomes the owner of it, but his proxy is saved as the owner of this vault in the _safeData mapping. This is due to the fact that the NFV implements an _afterTokenTransfer function that deploys the proxy if not already deployed and transfers the ownership.

After that we try to call the allowSAFE function to give permission to another user to our safe but since this function has the safeAllowed modifier the call reverts because we are not the owner of the vault and the proxy is.

Tools Used

Manual Review, Foundry, VS Code

Recommended Mitigation Steps

To solve this issue one thing we can do is just do a call in the execute function inside the ODProxy. This would then solve this issue but will create another one because other contracts(like the BasicActions) expect a delegatecall. Another step we can take towards solving this problem is by storing the user's address inside the _safeData and other mappings. This way we solve the msg.sender issue but the storage collision part is not solved. What I would recommend is to look at the overall structure again and to decide which of the above mentioned solutions suits you better and restructure other contracts to be able to work with the changes made.

Assessed type

call/delegatecall