Open Dollar - immeas's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L213-L221

Vulnerability details

Description

When there is a surplus in AccountingEngine anyone can handle that surplus by calling AccountingEngine::auctionSurplus. When called, it will start an auction with a certain amount of the surplus, and transfer the rest to extraSurplusReceiver. The split is decided depending on surplusTransferPercentage which is configured at creation.

The issue is that there is a mistake in the calculation of this percentage:

AccountingEngine::auctionSurplus:

File: src/contracts/AccountingEngine.sol

28:  uint256 internal constant ONE_HUNDRED_WAD = 100 * WAD;

198:  function auctionSurplus() external returns (uint256 _id) {
        // @audit here `surplusTransferPercentage` is treated to be in the range [0,1 WAD]
199:    if(_params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit();

        // ... deciding amount of surplus

        // @audit here `surplusTransferPercentage` is treated to be in the range [0,100 WAD]
212:    // auction surplus percentage
213:    if (_params.surplusTransferPercentage < ONE_HUNDRED_WAD) {
214:      _id = surplusAuctionHouse.startAuction({
215:        _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
216:        _initialBid: 0
217:      });
218:
219:      lastSurplusTime = block.timestamp;
220:      emit AuctionSurplus(_id, 0, _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage));
221:    }
222:
        // @audit here `surplusTransferPercentage` is treated to be in the range [0,1 WAD] again
223:    // transfer surplus percentage
224:    if (_params.surplusTransferPercentage > 0) {
225:      if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver();
226:
227:      safeEngine.transferInternalCoins({
228:        _source: address(this),
229:        _destination: extraSurplusReceiver,
230:        _rad: _params.surplusAmount.wmul(_params.surplusTransferPercentage)
231:      });
232:
233:      lastSurplusTime = block.timestamp;
234:      emit TransferSurplus(extraSurplusReceiver, _params.surplusAmount.wmul(_params.surplusTransferPercentage));
235:    }
236:  }

As you see, there is a conflict if surplusTransferPercentage is in the range [0,1 WAD] or [0,100 WAD].

Since the call reverts if the percentage is > 1 WAD on row 199, auctionSurplus will always auction at least 99 * surplusAmount and up to 100 * surplusAmount depending on the configured percentage.

Impact

When there is a surplus AccountingEngine::auctionSurplus will start auctions for 99 * surplusAmount more than intended.

Proof of Concept

Test in test/AccountingEngineTest.t.sol:

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

import {Test} from "forge-std/Test.sol";
import {AccountingEngine, IAccountingEngine} from '@contracts/AccountingEngine.sol';
import {ICommonSurplusAuctionHouse} from '@interfaces/ICommonSurplusAuctionHouse.sol';
import {ISAFEEngine} from '@interfaces/ISAFEEngine.sol';
import {Math, WAD, RAD} from '@libraries/Math.sol';

contract AccountingEngineTest is Test {

  AccountingEngine internal accountingEngine;

  address internal surplusAuction = makeAddr('SurplusAuction');
  address internal safeEngine = makeAddr('SafeEngine');

  function setUp() public {
    vm.mockCall(safeEngine, abi.encodeWithSelector(ISAFEEngine.approveSAFEModification.selector), new bytes(0));
    vm.mockCall(safeEngine, abi.encodeWithSelector(ISAFEEngine.settleDebt.selector), new bytes(0));
    vm.mockCall(safeEngine, abi.encodeWithSelector(ISAFEEngine.transferInternalCoins.selector), new bytes(0));

    accountingEngine = new AccountingEngine(
      safeEngine,
      surplusAuction,
      address(1),
      IAccountingEngine.AccountingEngineParams({
        surplusTransferPercentage: 0, // all should go to auction
        surplusDelay: 0,
        popDebtDelay: 0,
        disableCooldown: 0,
        surplusAmount: RAD, // 1 RAD
        surplusBuffer: 0,
        debtAuctionMintedTokens: 0,
        debtAuctionBidSize: 0
      })
    );
    accountingEngine.modifyParameters('extraSurplusReceiver', abi.encode(address(1)));
  }

  function testAuctionSurplus() public {
    // surplus is 1 RAD
    vm.mockCall(safeEngine, abi.encodeWithSelector(ISAFEEngine.coinBalance.selector), abi.encode(RAD));
    vm.mockCall(safeEngine, abi.encodeWithSelector(ISAFEEngine.debtBalance.selector), abi.encode(0));

    vm.mockCall(surplusAuction, abi.encodeWithSelector(ICommonSurplusAuctionHouse.startAuction.selector), abi.encode(0));

    // auction auctions of too much coins
    // should transfer 0, transfers 99 times the total
    // 100 WAD - 0 WAD = 100 WAD / WAD * 1 RAD = 100 RAD
    vm.expectCall(surplusAuction,abi.encodeCall(ICommonSurplusAuctionHouse.startAuction, (100 * RAD,0)));

    accountingEngine.auctionSurplus();
  }
}

Tools Used

Manual audit

Recommended Mitigation Steps

Consider using 1 WAD as 100% throughout the function and remove ONE_HUNDRED_WAD.

Assessed type

Math

Judge has assessed an item in Issue #175 as 2 risk. The relevant finding follows:

L-02 Initial values for GovernorSettings are very low ODGovernor is a OZ Governor with some plugins. It sets up its parameters in the constructor:

ODGovernor::constructor:

File: src/contracts/gov/ODGovernor.sol

41: GovernorSettings(1, 15, 0) These are in turn, initialVotingDelay=1, initialVotingPeriod=15 and initialProposalThreshold=0.

These values are very low, and initialProposalThreshold=0 can invite to spam. initialVotingPeriod=15 is very short as optimism uses L1 blocks with are just 12 seconds.

I assume these are test chain values, similar to the factories in uniswap and camelot relayers.

Recommendations Do not forget to change these values before going live.

Lines of code

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

Vulnerability details

Impact

CamelotRelayer will not work. As it uses Camelot pairs as if they were Uniswap v3 pools.

Proof of Concept

The code in CamelotRelayer::getResultWithValidity is copied over from UniV3Relayer. This is problematic because of these lines:

CamelotRelayer::getResultWithValidity:

File: src/contracts/oracles/CamelotRelayer.sol

70:    if (OracleLibrary.getOldestObservationSecondsAgo(camelotPair) < quotePeriod) {
71:      return (0, false);
72:    }
73:    // Consult the query with a TWAP period of quotePeriod
74:    (int24 _arithmeticMeanTick,) = OracleLibrary.consult(camelotPair, quotePeriod);

They treat the Camelot pair as a Uniswap v3 pool. The issue is that OracleLibrary.getOldestObservationSecondsAgo calls slot0 on the pool/pair:

75:        (, , uint16 observationIndex, uint16 observationCardinality, , , ) = IUniswapV3Pool(pool).slot0();

slot0 doesn't exist on the CamelotPairs:

https://arbiscan.io/address/0x6c0790a175dbce981301db099ef713794f41924a#code

Random example pair created by the CAMELOT_V3_FACTORY used in CamelotRelayer.

Similar issues continues in the code, getOldestObservationSecondsAgo also calls pool.observations and OracleLibrary.consult calls pool.observer which also don't exist on Camelot pairs.

The same issue exists in CamelotRelayer::read as well.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider changing the implementation of CamelotRelayer to use the TWAP functionality on CamelotPair instead of Uniswap.

Assessed type

Other

QA Report

Summary

Low

L-01 Someone can add a safe to their list that cannot be removed

ODSafeManager::addSAFE and removeSAFE behave a bit weird.

ODSafeManager::addSAFE:

File: src/contracts/proxies/ODSafeManager.sol

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

Here, a user can add any safe (created or not) to their list.

However in removeSAFE:

File: src/contracts/proxies/ODSafeManager.sol

242:  function removeSAFE(uint256 _safe) external safeAllowed(_safe) {
243:    SAFEData memory _sData = _safeData[_safe];
244:    _usrSafes[_sData.owner].remove(_safe);
245:    _usrSafesPerCollat[_sData.owner][_sData.collateralType].remove(_safe);
246:  }

Since it is safeAllowed(_safe) a user can only remove a safe from their list when they are the owner of it.

Hence a user could accidentally add a safe to their list which they are unable to remove.

These lists are only informational and have no effect on the logic of the protocol but they could cause UIs or other off-chain functions to look weird.

Recommendation

Consider changing so that instead of having removeSafe remove the safe from owner. Remove it from msg.sender and remove the check that msg.sender is owner.

L-02 Initial values for GovernorSettings are very low

ODGovernor is a OZ Governor with some plugins. It sets up its parameters in the constructor:

ODGovernor::constructor:

File: src/contracts/gov/ODGovernor.sol

41:    GovernorSettings(1, 15, 0)

These are in turn, initialVotingDelay=1, initialVotingPeriod=15 and initialProposalThreshold=0.

These values are very low, and initialProposalThreshold=0 can invite to spam. initialVotingPeriod=15 is very short as optimism uses L1 blocks with are just 12 seconds.

I assume these are test chain values, similar to the factories in uniswap and camelot relayers.

Recommendations

Do not forget to change these values before going live.

L-03 No way to change governor of Vault721

Vault721 is owned by governor. This address has the ability to change safeManager and nftRenderer.

There is however no way for governor to change its own address. If governance for whatever reason needs to migrate to a new address this makes that move problematic as that would lock those two values in Vault721

Recommendation

Consider adding a way to change the governor address of Vault721.

Recommendations

R-01 _isNotProxy is confusingly named

Vault721::_isNotProxy

File: src/contracts/proxies/Vault721.sol

154:  function _isNotProxy(address _user) internal view returns (bool) {
155:    return _userRegistry[_user] == address(0) || ODProxy(_userRegistry[_user]).OWNER() != _user;
156:  }

This tests if _user has a registered proxy. However, the name _isNotProxy implies that _user should be a proxy.

Consider changing the name of _isNotProxy to _hasNoProxy. This better conveys the relationship between the user and the proxy.

R-02 Avoid negations, especially double negations

Vault721::_isNotProxy

File: src/contracts/proxies/Vault721.sol

154:  function _isNotProxy(address _user) internal view returns (bool) {
155:    return _userRegistry[_user] == address(0) || ODProxy(_userRegistry[_user]).OWNER() != _user;
156:  }

this is used in these three locations:

File: src/contracts/proxies/Vault721.sol

78:     if (!_isNotProxy(msg.sender)) revert ProxyAlreadyExist();

86:     if (!_isNotProxy(_user)) revert ProxyAlreadyExist();

192:      if (_isNotProxy(to)) {
193:        proxy = _build(to);
194:      } else {
195:        proxy = payable(_userRegistry[to]);
196:      }

The double negation !_isNotProxy is hard to grasp.

Consider switching the function to check if there is a proxy and rename it to _isProxy (see above issue about naming though). Then turn around the if-statement in _afterTokenTransfer.

R-03 Calculation is repeated in same code block

_params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage) is done on both line 215 and 220 in AccountingEngine::auctionSurplus

File: src/contracts/AccountingEngine.sol

213:    if (_params.surplusTransferPercentage < ONE_HUNDRED_WAD) {
214:      _id = surplusAuctionHouse.startAuction({
215:        _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
216:        _initialBid: 0
217:      });
218:
219:      lastSurplusTime = block.timestamp;
220:      emit AuctionSurplus(_id, 0, _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage));
221:    }

Consider rewriting it to avoid repeating calculations:

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

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

R-04 Unnecessary OWNER check

Vault721::_isNotProxy

File: src/contracts/proxies/Vault721.sol

154:  function _isNotProxy(address _user) internal view returns (bool) {
155:    return _userRegistry[_user] == address(0) || ODProxy(_userRegistry[_user]).OWNER() != _user;
156:  }

When creating a proxy, _userRegistry[_user] is written with the created proxy. In ODProxy, OWNER is immutable hence cannot be changed.

The check ODProxy(_userRegistry[_user]).OWNER() != _user is therefore unnecessary since if there is a proxy in _userRegistry the _user must be owner of it.

Consider removing the extra, unnecessary, check as it is confusing and can fool you to believe OWNER is mutable.

Informational / Non-critical

NC-01 Confusing documentation

The documentation for ODSafeManager::openSAFE says:

IODSafeManager::openSAFE

File: src/interfaces/proxies/IODSafeManager.sol

122:  /**
123:   * @notice Open a new safe for a user address
124:   * @param  _cType Bytes32 representation of the collateral type
125:   * @param  _usr Address of the user to open the safe for
126:   * @return _id Id of the new SAFE
127:   */
128:  function openSAFE(bytes32 _cType, address _usr) external returns (uint256 _id);

ODSafeManager::openSAFE:

File: src/contracts/proxies/ODSafeManager.sol

117:  /// @inheritdoc IODSafeManager
118:  function openSAFE(bytes32 _cType, address _usr) external returns (uint256 _id) {
119:    if (_usr == address(0)) revert ZeroAddress();
...
129:    vault721.mint(_usr, _safeId);
130:
131:    emit OpenSAFE(msg.sender, _usr, _safeId);
132:    return _safeId;
133:  }

And the parameter for who the safe is for is _usr throughout the function. However, the parameter _usr from ODSafeManager is required to be _proxy in Vault721::mint:

File: src/contracts/proxies/Vault721.sol

90:   /**
91:    * @dev mint can only be called by the SafeManager
92:    * enforces that only ODProxies call `openSafe` function by checking _proxyRegistry
93:    */
94:   function mint(address _proxy, uint256 _safeId) external {
95:     require(msg.sender == address(safeManager), 'V721: only safeManager');
        // @audit what is passed as `_usr` from safe manager is referred to as `_proxy` here
96:     require(_proxyRegistry[_proxy] != address(0), 'V721: non-native proxy');
97:     address _user = _proxyRegistry[_proxy];
98:     _safeMint(_user, _safeId);
99:   }

This is confusing, consider correcting the documentation for openSAFE to reflect that the user needs to open a proxy with Vault721 before and then call openSAFE from that proxy.

NC-02 Removed whitspace unaligns code

AccountingEngine::auctionSurplus

File: src/contracts/AccountingEngine.sol

        // missing whitespace
199:    if(_params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit();
200:    if (_params.surplusAmount == 0) revert AccEng_NullAmount();

A whitespace between if and ( is missing, unaliging the code with the rest of the if-statements