Open Dollar - perseus's results

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

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

Lines of code

https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/gov/ODGovernor.sol#L41

Vulnerability details

Impact

Governance system is initialised with 1 block of initial voting delay and 15 blocks for the duration of the voting period. These parameters are inadequate for meaningful governance process as they allow proposals to be created and executed after 3-4 mins on Arbitrum. This short voting period does not allow end users to react in case of undesired system changes, while attackers may potentially compromise system by proposing, voting for, and executing malicious proposal in short time span before others may even notice it.

Based on the values it seems that incorrect assumption is that unit for these params is days while the actual unit is a block.

Proof of Concept

Following is the initilization of ODGovernor with mentioned parameters.

  constructor(
    address _token,
    TimelockController _timelock
  )
    Governor('ODGovernor')
    GovernorSettings(1, 15, 0)
    GovernorVotes(IVotes(_token))
    GovernorVotesQuorumFraction(3)
    GovernorTimelockControl(_timelock)
  {}

Tools Used

Manual review

Recommended Mitigation Steps

Set initial GovernorSettings parameters to a more meaningful values.

Assessed type

Governance