search

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

10 stars 7 forks source link

Updating safeManager reference in Vault721 will brick transfer of safes #433

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/proxies/ODSafeManager.sol#L20

Vulnerability details

Impact

Updating safeManager reference in Vault721 will brick safe transfers since the state of the new ODSafeManager instance won't have corresponding data. In addition, it is not clear how it would be possible to achieve seamless migration as particular safeHandler instance grants safe modification permission within SafeEngine only to the single/original ODSafeManager instance and cannot be updated afterwards since there is no functionality for that in SafeHandler.sol contract.

It seems that if updates to the implementation are expected ODSafeManager should be a proxy contract.

Proof of Concept

Tools Used

Manual review.

Recommended Mitigation Steps

Consider making ODSafeManager reference in Vault721 immutable. Make ODSafeManager upgradeable contract.

Assessed type

Upgradable

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 #37

c4-judge commented 1 year ago

MiloTruck marked the issue as not a duplicate

c4-judge commented 1 year ago

MiloTruck marked the issue as duplicate of #233

c4-judge commented 12 months ago

MiloTruck marked the issue as partial-25

MiloTruck commented 12 months ago

Partial credit as the report has highlighted the bug, but didn't provide sufficient explanation as to what the bug is.

c4-judge commented 12 months ago

MiloTruck changed the severity to 2 (Med Risk)

c4-judge commented 12 months ago

MiloTruck marked the issue as satisfactory