Open Dollar - xAriextz's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L49-L53 Example of function that won't work:https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L105-L109

Vulnerability details

Impact

A lot of functions from ODSafeManager won't be usable, limiting the usecases of the project.

Proof of Concept

ODSafeManager functions are thought for being called from an intermediary contract like BasicActions. The problem is that some functions like allowSAFE, protectSAFE, etc must be called directly from the Proxy because there is no intermediary contract for this. This functions have the safeAllowed modifier, which won't work since when calling directly from the ODProxy's delegatecall the msg.sender will be the EOA of the user, reveverting the operation.

Example: user wants to allow another account to manage his positions, so he tries delegatecalling from his ODProxy to allowSAFE in ODSafeManager for this. The safeAllowed modifier will revert, since the msg.sender will be the user's address instead of the ODProxi's address. Users won't have access to all the functions in ODSafeManager that are not called from BasicActions.

You can add this piece of code to a test of the AnvilFrok.t.sol. If you call it, you can see that it will revert in the safeAllowed modifier.

function allowSafe(address _proxy, uint256 _safeId) public {
bytes memory payload = abi.encodeWithSelector(safeManager.allowSAFE.selector, _safeId, makeAddr("RandomeAddress"), 1);
bytes memory safeData = ODProxy(_proxy).execute(address(safeManager), payload);
}

Tools Used

Manual review and Foundry

Recommended Mitigation Steps

Add functions in BasicActions for calling all the ODSafeManager functions.

Assessed type

DoS

Lines of code

Wrong address asignment: https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/CamelotRelayer.sol#L20

Vulnerability details

Impact

The address being used for the Camelot Factory is the testnet address, which will make the deployment of CamelotRelayer to revert.

Proof of Concept

As stated in the docs of the contest: Protocol will be deployed to Arbitrum One. Nevertheless, the address being assigned in CamelotRelayer.sol for the Camelot Factory is in fact the testnet address. This will make the deployment to revert since camelotPair will be assigned to the address(0), making the constructor revert.

Tools Used

Manual review

Recommended Mitigation Steps

Assign to Camelot Factory the Arbitrum One mainnet address instead of the one from testnet.

Assessed type

Error

Lines of code

Wrong address asignment: https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/oracles/UniV3Relayer.sol#L18

Vulnerability details

Impact

The address being used for the Uniswap Factory is the testnet address, which will make the deployment of UniswapRelayer to revert.

Proof of Concept

As stated in the docs of the contest: Protocol will be deployed to Arbitrum One. Nevertheless, the address being assigned in UniswapRelayer.sol for the Uniswap Factory is in fact the testnet address. This will make the deployment to revert since uniV3Pool will be assigned to the address(0), making the constructor revert.

Tools Used

Manual review

Recommended Mitigation Steps

Assign to Uniswap Factory the Arbitrum One mainnet address instead of the one from testnet.

Assessed type

Error

Lines of code

afterTokenTransfer of the Vault: https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L187-L199 Transferring the ownership of a Safe: https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L136-L152 Mappings of approvals that are not cleared: https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L39-L41

Vulnerability details

Impact

When transferring a Vault from one user to another (for example a Vault being bought) the ownership of the safe is also transferred. The problem is that safeCan mapping is not cleared. This means that the addresses that had permission for calling the functions of ODSafeManager.sol for the previous owner are not cleared. This means that if a user rebuys a Vault someday, the safeCan will be the same one as the time he sold the Vault, which will lead to problems like funds being stolen.

Proof of Concept

Example 1: Alice owns a Vault. Alice allows Charlie to manage her positions by using allowSAFE(). Alice sells the Vault to Bob, but the safeCan is not cleared. Alice and Charlie get into a discussion, and do not trust each other anymore. This will also happen if Charlie's wallet gets compromised or whatever. (We can think about Charlie as a protocol also). Alice decides to buy that Vault again. Now, Charlie has access for managing Alice's positions again, even if Alice just bought it. Charlie can decide to use functions in ODSafeManager.sol such as transferCollateral() and transferInternalCoins() since he still has permissions. Even if Alice tries disallowing him from managing her positions, Charlie can frontrun it transferring collateral and internal coins prior to losing the allowance.

Even if in this example Charlie becomes a "malicious" user when Alice doesn't own the NFV, this doesn't need to be the case. For instance Charlie can be a DAO that manages users funds regarding some rules. Even if Alice leaves the DAO and doesn't want it to have an allowance, the DAO can still manage her funds (on purpose or not) before she clears the allowances.

Example 2: Alice owns a Vault. Alice allows Bob to manage her positions by using allowSAFE(). Alice sells the Vault to Bob, but the safeCan is not cleared. Now, Alice decides to buy the Vault from Bob again. Alice doesn't want Bob to still manage her positions, so she decides to disallow him. Bob realizes this and that he is still allowed, so he frontruns Alice's transaction transferring collateral and internal coins.

The big problem in both cases is that Alice will never be safe to rebuy her NFV in case the allowances she did are not longer of her pleasure, since they can steal her funds before she can disallow them.

Tools Used

Manual review

Recommended Mitigation Steps

Clear the allowances of the safeCan mapping like OZ does with approvals when transferring an ERC721.

When transferring ERC721 tokens there is the following code (in OZ's implementation):

// Clear approvals
delete _tokenApprovals[tokenId]; 

Something similar could be added with safeCan

Assessed type

Access Control

Lines of code

affected line: https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L163

Vulnerability details

Impact

The _build() function deploys a new ODProxy contract using the create, where the address derivation depends only on the Vault721 nonce. Arbitrum is suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation.

Users can deploy a ODProxy and it can receive funds. If a reorg happens, another user could deploy the ODProxy before, stealing funds.

Re-orgs can happen in all EVM chains and as confirmed the contracts will be deployed on Arbitrum.

Proof of Concept

Attack Scenario Imagine that Alice deploys a new ODProxy, and then sends funds to it. Bob sees that the network block reorg happens and calls build(). Thus, it creates ODProxy with an address to which Alice sends funds. Then Alices’ transactions are executed and Alice transfers funds to Bob’s controlled ODProxy.

This is a Medium severity issue that has been referenced from below Code4rena reports: https://code4rena.com/reports/2023-01-rabbithole/#m-01-questfactory-is-suspicious-of-the-reorg-attack https://code4rena.com/reports/2023-04-frankencoin#m-14-re-org-attack-in-factory

Tools Used

Manual review

Recommended Mitigation Steps

Deploy such contracts via create2 with salt that includes the future owner of the ODProxy and a nonce. Something like this will work:

// Calculate the salt for the owner, incrementing their nonce in the process
bytes32 _salt = keccak256(abi.encode(_owner, nonces[_owner]++));
// Create the new proxy
_proxy = payable(address(new ODProxy{salt: _salt}(_owner)));

Assessed type

Other