code-423n4 / 2023-10-opendollar-findings

10 stars 7 forks source link

initializeManager() & initializeRenderer() can be frontrun by mev bots and brick the protocol temporarily #328

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L56 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L63

Vulnerability details

Impact

There is no access control while initialising safe manager & nftRenderer so mev bots can monitor the deployment of Vault721 contract and initialise both safe manager & nftRenderer with malicious addresses and brick the protocol until the governer can update the addresses

Proof of Concept

The mev bot can have the following contracts set as safe manager and NFT renderer

contract MaliciousManager {}

contract MaliciosNFTRenderer {}

now this would mean that no one would be able to mint a safe since there is no transferSAFEOwnership method until the governer updates the addresses

Tools Used

manual review

Recommended Mitigation Steps

add access control in the initialize methods and hence add onlyGovernor modifier there too so avoid any sort of hindrance to the protocol

Assessed type

Access Control

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #16

c4-judge commented 1 year ago

MiloTruck changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

MiloTruck marked the issue as grade-b