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

10 stars 7 forks source link

Missing functionality required to allow others access to safe #421

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

Vulnerability details

Impact

Missing functionality prevents ODProxy from granting other accounts access to the safe.

In order to grant access to the particular safe for which specific instance of ODProxy is the owner, it is necessary to call allowSAFE() on ODSafeManager. This method initially can only be invoked by the safe owner or the corresponding instance of ODProxy.

System is designed so that ODProxy uses functionality in BasicActions.sol and CommonActions.sol to interact with the ODSafeManager (by relying on delegatecall). However, current functionality in BasicActions and CommonActions does not contain corresponding functionality for invoking allowSAFE().

Also calling directly ODSafeManager.allowSAFE() through ODProxy does not work since that call would be executed in the context of ODProxy (due to delegatecall invocation).

For users to successfully invoke allowSAFE() they would need to deploy or use some other already deployed contract with extra functionality that wraps interaction with ODSafeManager.allowSAFE(). In this case delegatecall will be properly propagated and updates in ODSafeManager will be properly executed in the context of ODSafeManager and not of ODProxy.

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

Add extra functionality to the BasicActions.sol or document how end users should deploy extra functionality to interact with all ODSafeManager contract features.

Assessed type

call/delegatecall

c4-pre-sort commented 1 year ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #380

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

c4-judge commented 1 year ago

MiloTruck marked the issue as satisfactory