Open Dollar - T1MOH's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/AccountingEngine.sol#L215

Vulnerability details

Impact

surplusTransferPercentage is meant to be set to [0; WAD] according to comments. However it is used in calculations where 100% means 100 WAD. As a result, It will try to sell amount 100x higher than intended.

Surplus transfers and auctions will be 100 times higher, also transfer percentage is incorrect: if set surplusTransferPercentage to 0.5 WAD (50%), it will transfer 0.5% to protocol and 99.5% will sell on auction

Proof of Concept

Here it says that WAD is 100% https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/interfaces/IAccountingEngine.sol#L90-L97

/// @notice Throws when surplus surplusTransferPercentage is great than WAD (100%)
  error AccEng_surplusTransferPercentOverLimit();

  // --- Structs ---

  struct AccountingEngineParams {
    // percent of the Surplus the system transfers instead of auctioning [0/100]
    uint256 surplusTransferPercentage;
    ...
  }

In this calculation ONE_HUNDRED_WAD is used instead WAD, selling amount 100x higher than intended https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/AccountingEngine.sol#L215

    if (_params.surplusTransferPercentage < ONE_HUNDRED_WAD) {
      _id = surplusAuctionHouse.startAuction({
        _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
        _initialBid: 0
      });

Tools Used

Manual Review

Recommended Mitigation Steps

Update to use WAD:

  function auctionSurplus() external returns (uint256 _id) {
    if(_params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit();
    if (_params.surplusAmount == 0) revert AccEng_NullAmount();
    if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver();
    if (block.timestamp < lastSurplusTime + _params.surplusDelay) revert AccEng_SurplusCooldown();

    uint256 _coinBalance = safeEngine.coinBalance(address(this));
    uint256 _debtBalance = safeEngine.debtBalance(address(this));
    (_coinBalance, _debtBalance) = _settleDebt(_coinBalance, _debtBalance, _unqueuedUnauctionedDebt(_debtBalance));

    if (_coinBalance < _debtBalance + _params.surplusAmount + _params.surplusBuffer) {
      revert AccEng_InsufficientSurplus();
    }

    // auction surplus percentage
-   if (_params.surplusTransferPercentage < ONE_HUNDRED_WAD) {
+   if (_params.surplusTransferPercentage < WAD) {
      _id = surplusAuctionHouse.startAuction({
-       _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
+       _amountToSell: _params.surplusAmount.wmul(WAD - _params.surplusTransferPercentage),
        _initialBid: 0
      });

      lastSurplusTime = block.timestamp;
-     emit AuctionSurplus(_id, 0, _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage));
+     emit AuctionSurplus(_id, 0, _params.surplusAmount.wmul(WAD - _params.surplusTransferPercentage));
    }

    // transfer surplus percentage
    if (_params.surplusTransferPercentage > 0) {
      if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver();

      safeEngine.transferInternalCoins({
        _source: address(this),
        _destination: extraSurplusReceiver,
        _rad: _params.surplusAmount.wmul(_params.surplusTransferPercentage)
      });

      lastSurplusTime = block.timestamp;
      emit TransferSurplus(extraSurplusReceiver, _params.surplusAmount.wmul(_params.surplusTransferPercentage));
    }
  }

Assessed type

Invalid Validation

Lines of code

https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/AccountingEngine.sol#L181

Vulnerability details

Impact

Before starting debtAuction, current implementation checks that there is enough debt in system. However this check is performed before debt settlement. This check bypass will make totalOnAuctionDebt higher than actual debtBalance. As a result, core functions _settleDebt() and unqueuedUnauctionedDebt will revert. As well as cancelAuctionedDebtWithSurplus which means that debtAuction can't be settled.

It will introduce uncertain consequences. Because that auction with non-existant debt won't be settled in time, which means it will be restarted, every time supplying more and more tokens to mint. Somewhen it will be settled, potentially long time in future, thus minting huge amount of internalCoin to higherBidder.

Proof of Concept

Suppose following scenario:

1. debtBalance = 300, coinBalance = 0, totalQueuedDebt = 0, totalOnAuctionDebt = 0. It means there is no bad debt in system, it shouldn't start DebtAuction
2. debtAuctionBidSize = 300. auctionDebt() is called. It firstly ensures that debtAuctionBidSize <= debtBalance. And secondly increases totalOnAuctionDebt. And starts Auction

  function auctionDebt() external returns (uint256 _id) {
    if (_params.debtAuctionBidSize == 0) revert AccEng_DebtAuctionDisabled();

    uint256 _coinBalance = safeEngine.coinBalance(address(this));
    uint256 _debtBalance = safeEngine.debtBalance(address(this));

    if (_params.debtAuctionBidSize > _unqueuedUnauctionedDebt(_debtBalance)) revert AccEng_InsufficientDebt();

@>  (_coinBalance, _debtBalance) = _settleDebt(_coinBalance, _debtBalance, _coinBalance);
@>  totalOnAuctionDebt += _params.debtAuctionBidSize;

    _id = debtAuctionHouse.startAuction({
      _incomeReceiver: address(this),
      _amountToSell: _params.debtAuctionMintedTokens,
      _initialBid: _params.debtAuctionBidSize
    });
    ...
  }

3. Now totalOnAuctionDebt is greater than debtBalance. Therefore functions like _unqueuedUnauctionedDebt(), _settleDebt() will revert due to underflow. As well as cancelAuctionedDebtWithSurplus because it tries to repay more debt than AccountingEngine actually has. And it will revert until system has enough debt

Tools Used

Manual Review

Recommended Mitigation Steps

Change check order:

  function auctionDebt() external returns (uint256 _id) {
    ...

-   if (_params.debtAuctionBidSize > _unqueuedUnauctionedDebt(_debtBalance)) revert AccEng_InsufficientDebt();

    (_coinBalance, _debtBalance) = _settleDebt(_coinBalance, _debtBalance, _coinBalance);
    totalOnAuctionDebt += _params.debtAuctionBidSize;
+   if (_params.debtAuctionBidSize > _unqueuedUnauctionedDebt(_debtBalance)) revert AccEng_InsufficientDebt();

    ...
  }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Function ODSafeManager.addSAFE() should be called when safeId was previously removed from mappings _usrSafes and _usrSafesPerCollat. ODSafeManager.addSAFE() must be called via delegateCall to BasicActions, because it uses msg.sender. However there are no such functions addSAFE() and removeSAFE() in BasicActions. It's obviously not intended because user must deploy his own library to delegateCall from proxy to call them

Proof of Concept

Note that msg.sender is used. To msg.sender be proxy, proxy must delegateCall to another function which calls addSAFE(). That's why BasicActions.sol exists. However BasicActions.sol doesn't call addSAFE() and removeSAFE()

  function addSAFE(uint256 _safe) external {
    SAFEData memory _sData = _safeData[_safe];
    _usrSafes[msg.sender].add(_safe);
    _usrSafesPerCollat[msg.sender][_sData.collateralType].add(_safe);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Add calls to addSAFE() and removeSAFE() in BasicActions.sol

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

3 minutes to vote is obviously too low to vote against proposal. Especially when VotingDelay set to 1 block

Proof of Concept

votingDelay is set to 1 block, votingPeriod is set to 15 blocks: https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/gov/ODGovernor.sol#L41

abstract contract GovernorSettings is Governor {
    ...

    constructor(
        uint256 initialVotingDelay,
        uint256 initialVotingPeriod,
        uint256 initialProposalThreshold
    ) {
        _setVotingDelay(initialVotingDelay);
        _setVotingPeriod(initialVotingPeriod);
        _setProposalThreshold(initialProposalThreshold);
    }

    ...
}

Tools Used

Manual Review

Recommended Mitigation Steps

Set votingPeriod and votingDelay to real values

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

Current implementation allows approved user to give permissions. As a result, malicious approved user can't be deleted from approved, because always can give approval to another malicious address

Proof of Concept

safeAllowed() checks that msg.sender is owner or approved:

  function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {
    address _owner = _safeData[_safe].owner;
    safeCan[_owner][_safe][_usr] = _ok;
    emit AllowSAFE(msg.sender, _safe, _usr, _ok);
  }

  modifier safeAllowed(uint256 _safe) {
    address _owner = _safeData[_safe].owner;
    if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed();
    _;
  }

So when user decides to remove approval, malicious user can frontrun and give permission to another address

Tools Used

Manual Review

Recommended Mitigation Steps

Permit only owner to give permission:

- function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {
+ function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external {
    address _owner = _safeData[_safe].owner;
+   if (msg.sender != _owner) revert SafeNotAllowed();
    safeCan[_owner][_safe][_usr] = _ok;
    emit AllowSAFE(msg.sender, _safe, _usr, _ok);
  }

Assessed type

MEV

Lines of code

https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/oracles/CamelotRelayer.sol#L68-L101

Vulnerability details

Impact

CamelotRelayer reverts when tries to get price from Camelot Pool. It uses Uniswap's OracleLibrary, however Camelot Pool is not compatible with it.

Proof of Concept

OracleLibrary's functions consult() and getOldestObservationSecondsAgo() are utilized. They call functions observe() and slot0() on CamelotPool contract

    function consult(address pool, uint32 secondsAgo)
        internal
        view
        returns (int24 arithmeticMeanTick, uint128 harmonicMeanLiquidity)
    {
        ...

@>      (int56[] memory tickCumulatives, uint160[] memory secondsPerLiquidityCumulativeX128s) = IUniswapV3Pool(pool)
            .observe(secondsAgos);

        ...
    }

    function getOldestObservationSecondsAgo(address pool) internal view returns (uint32 secondsAgo) {
@>      (, , uint16 observationIndex, uint16 observationCardinality, , , ) = IUniswapV3Pool(pool).slot0();
        
        ...
    }

However CamelotPool doesn't have such functions. Insert this file to test/

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.19;

import "./e2e/Common.t.sol";
import "../src/contracts/oracles/CamelotRelayer.sol";

import "forge-std/console.sol";
import "forge-std/Test.sol";

contract MyTest is Test {

    function test_CamelotRelayerDontWork() public {
        // fork Arb Mainnet
        vm.createSelectFork("https://arbitrum-one.publicnode.com");

        // https://docs.camelot.exchange/contracts/amm-v2/factory
        // Get random pair - index 2
        ICamelotPair camelotPair = ICamelotPair(ICamelotFactory(0x6EcCab422D763aC031210895C81787E87B43A652).allPairs(2));
        

        // Token addresses from pair https://arbiscan.io/address/0x9Fa7314525d5C706213468220C7c9820Cfe629cb
        CamelotRelayer camelotRelayer = new CamelotRelayer(camelotPair.token0(), camelotPair.token1(), 100);

        // reverts because Camelot pair doesn't have `slot0()` and `observe()`
        camelotRelayer.getResultWithValidity();
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Develop custom OracleLibrary for CamelotPool

Assessed type

Oracle

1. Low. Consider disabling ability to deploy proxy for proxy

https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/proxies/Vault721.sol#L154-L156

Perform additional check to ensure that user is not registered proxy

  function _isNotProxy(address _user) internal view returns (bool) {
-   return _userRegistry[_user] == address(0) || ODProxy(_userRegistry[_user]).OWNER() != _user;
+   return (_userRegistry[_user] == address(0) || ODProxy(_userRegistry[_user]).OWNER() != _user) && 
+     _proxyRegistry[_user] == address(0);
  }