Re-org attack when deploying ODProxies

[c4-submissions]

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

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #21

[c4-judge]

MiloTruck marked the issue as not a duplicate

[c4-judge]

MiloTruck marked the issue as duplicate of #410

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[c4-judge]

MiloTruck marked the issue as grade-b