Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deployRentalSafe(...) could be DOSed #553

Closed
c4-bot-2 opened this issue Jan 18, 2024 · 4 comments
Closed

deployRentalSafe(...) could be DOSed #553

c4-bot-2 opened this issue Jan 18, 2024 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-443 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-2
Copy link
Contributor

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-bot-2 c4-bot-2 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jan 18, 2024
c4-bot-2 added a commit that referenced this issue Jan 18, 2024
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #443

@c4-judge
Copy link
Contributor

0xean marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jan 28, 2024
@c4-judge
Copy link
Contributor

0xean changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed satisfactory satisfies C4 submission criteria; eligible for awards labels Feb 1, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Feb 1, 2024

0xean marked the issue as grade-c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-443 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants