code-423n4 / 2024-01-renft-findings

2 stars 0 forks source link

deployRentalSafe(...) could be DOSed #553

Closed c4-bot-2 closed 10 months ago

c4-bot-2 commented 10 months ago

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Factory.sol#L138-L194

Vulnerability details

Impact

Some malicious users could DoS the deployRentalSafe(...) function by front running transactions.

Proof of Concept

The deployRentalSafe(...) is making a call the the (SafeProxyFactory)[https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Factory.sol#L180-L186]. This factory is open to anyone and uses the CREATE2 opcode to deploy a new contract. This opcode allows to deploy a contract at a specific address. If the address is already taken, the transaction will fail.

The deterministic address is computed using the salt passed to it. As you can see its calculation is uint256(keccak256(abi.encode(STORE.totalSafes() + 1, block.chainid))). Someone watching the mempool could see the transaction, compute the salt and deploy a contract at the same address before the transaction is mined.

Doing it will make the 'legit' deployRentalSafe(...) transaction fail preventing any user from deploying Safes and use the protocol.

Recommendation

Deploy the Safe directly in the deployRentalSafe(...) function and using the msg.sender in the salt calculation making it impossible to front run. Kind of how it is done in the Create2Deployer.sol.

Assessed type

DoS

c4-pre-sort commented 10 months ago

141345 marked the issue as duplicate of #443

c4-judge commented 10 months ago

0xean marked the issue as satisfactory

c4-judge commented 10 months ago

0xean changed the severity to QA (Quality Assurance)

c4-judge commented 9 months ago

0xean marked the issue as grade-c