Open Dollar - btk's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L22

Vulnerability details

Impact

Calling allowSAFE() directly with a proxy using execute will result in no changes in ODSafeManager, because delegatecall does not alter the context of the target contract.

Proof of Concept

[!IMPORTANT] Dev comment: You don't need a basic action to call allowSafe, you should be able to call it directly with a proxy using execute

Dev is assuming that you can call allowSAFE() directly with a proxy using execute, but that can't happend due to the fact that delegatecall don't change the context of the target contract and thus nothing will change in ODSafeManager and users will not be able to call allowSAFE(). Same thing for the allowHandler() function.

[!NOTE] Every function with safeAllowed() or handlerAllowed() modifier will be affected by this vulnerability.

For more information please visit: https://docs.soliditylang.org/en/v0.4.21/introduction-to-smart-contracts.html#delegatecall-callcode-and-libraries

Tools Used

Manual review

Recommended Mitigation Steps

We recommend adding a basic action to allow allowSAFE() in the BasicActions contract.

Assessed type

call/delegatecall

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/gov/ODGovernor.sol#L41

Vulnerability details

Impact

Voting delay doesn't work properly on Arbitrum because it relies on the block.number which doesn't provide users enough time to obtain or delegate voting power before a proposal vote ends.

Proof of Concept

In ODGovernor, this function measures the delay between proposal creation and the start of voting:

  function votingDelay() public view override(IGovernor, GovernorSettings) returns (uint256) {
    return super.votingDelay();
  }

And this function represents the delay between the vote start and vote end:

  function votingPeriod() public view override(IGovernor, GovernorSettings) returns (uint256) {
    return super.votingPeriod();
  }

Both this function enforces a block delay. This structure is problematic for use on Arbitrum. According to Arbitrum Docs block.number returns the most recently synced L1 block.number. Once per minute the block.number in the Sequencer is synced to the actual L1 block.number (This may introduce potential vulnerabilities in the governance system):

[!IMPORTANT]
As a general rule, any timing assumptions a contract makes about block numbers and timestamps should be considered generally reliable in the longer term (i.e., on the order of at least several hours) but unreliable in the shorter term (minutes).

And the fact that Arbitrum block time is 2-second. Thus voters will not have enough time to vote, or delegate it, before the voting of a proposal ends.

This is the current config:

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

• initialVotingDelay == 2 seconds
• initialVotingPeriod == 30 seconds
• initialProposalThreshold == 0 seconds

Which doesn't provide users enough time to obtain or delegate voting power before a proposal vote ends.

Tools Used

Manual review

Recommended Mitigation Steps

Replace block.number with block.timestamp and increase the above variables to provide voters with sufficient time to take action.

Assessed type

Timing

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/CamelotRelayer.sol#L20 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/UniV3Relayer.sol#L18

Vulnerability details

Impact

Protocol will not work on arbitrum due to hardcoded _UNI_V3_FACTORY and _CAMELOT_FACTORY contract addresses

Proof of Concept

_CAMELOT_FACTORY in the CamelotRelayer is hardcoded as:

address constant GOERLI_CAMELOT_V3_FACTORY = 0x5Cd40c7E21A15E7FC2503Fffd77cF70c60628F6C; // AlgebraFactory

But this address is the AlgebraFactory address in goerli arbitrium, but it is not on arbitrum:

https://arbiscan.io/address/0x5Cd40c7E21A15E7FC2503Fffd77cF70c60628F6C

Note: Same for _UNI_V3_FACTORY

https://arbiscan.io/address/0x4893376342d5D7b3e31d4184c08b265e5aB2A3f6

Tools Used

Manual review

Recommended Mitigation Steps

Use valid addresses for _UNI_V3_FACTORY and _CAMELOT_FACTORY

Assessed type

Other