Open Dollar - Krace's results

Lines of code

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

Vulnerability details

Impact

IAccountingEngine defines WAD as equivalent to 100%, as indicated by the comment:

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

However, within the function auctionSurplus, the code uses the constant ONE_HUNDRED_WAD to calculate the _amountToSell:

  uint256 internal constant ONE_HUNDRED_WAD = 100 * WAD;

This misuse of WAD and ONE_HUNDRED_WAD enforces a requirement that the _amountToSell must be greater than 9900% of _params.surplusAmount, thereby directly impacting the normal contract logic.

Proof of Concept

auctionSurplus stipulates that _params.surplusTransferPercentage must be less than or equal to WAD.

  function auctionSurplus() external returns (uint256 _id) {
    if(_params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit();

However, during the calculation of _amountToSell, the code employs ONE_HUNDRED_WAD to compute the percentage. Because _params.surplusTransferPercentage is less than or equal to WAD, the result of ONE_HUNDRED_WAD - _params.surplusTransferPercentage must be greater than or equal to 99 * WAD. Given that WAD is defined as 100%, this outcome exceeds 9900%.

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

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

  function wmul(uint256 _x, uint256 _y) internal pure returns (uint256 _wmul) {
    return (_x * _y) / WAD;
  }

Tools Used

vscode

Recommended Mitigation Steps

To address this issue, it is advisable to utilize ONE_HUNDRED_WAD as equivalent to 100% both for verifying the surplusTransferPercentage and calculating the _amountToSell.

Assessed type

Math

Low Risk

Non-Critical

Low Risk

[L-01] Precision loss in auctionSurplus

auctionSurplus() will calculate the _rad by: surplusAmount * surplusTransferPercentage / WAD. But there is no checks to ensure that the result of multiplication is bigger than WAD, so precision loss may occur when the owner wrongly sets the _params.

    if (_params.surplusTransferPercentage > 0) {
      if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver();

      safeEngine.transferInternalCoins({
        _source: address(this),
        _destination: extraSurplusReceiver,
        _rad: _params.surplusAmount.wmul(_params.surplusTransferPercentage)  // @audit if surplusTransferPercentage * surplusAmount is less than WAD, precisionl will loss
      });

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

  function wmul(uint256 _x, uint256 _y) internal pure returns (uint256 _wmul) {
    return (_x * _y) / WAD;
  }

[L-02] Frontrun attack in Vault721

initializeManager and initializeRenderer can be invoked by anyone when the address is not set.

Attack could frontrun this function to set the corresponding address.

  /**
   * @dev initializes SafeManager contract
   */
  function initializeManager() external {
    if (address(safeManager) == address(0)) _setSafeManager(msg.sender);
  }

  /**
   * @dev initializes NFTRenderer contract
   */
  function initializeRenderer() external {
    if (address(nftRenderer) == address(0)) _setNftRenderer(msg.sender);
  }

Non-critical

[NC-01] Redundant checks in auctionSurplus

auctionSurplus() has a redundant checks at line 201 and line 225. And the second check is useless.

  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(); //@audit redundant
    [....]
    // transfer surplus percentage
    if (_params.surplusTransferPercentage > 0) {
      if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver(); //@audit redundant

[NC-02] Useless check in auctionSurplus

This check is useless because there is a more strict check at the begin of function auctionSurplus.

  function auctionSurplus() external returns (uint256 _id) {
    if(_params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit();  //@audit this requires that surplusTransferPercentage <= WAD
    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) {
    //@audit this is useless because surplusTransferPercentage is strictly less than WAD, ONE_HUNDRED_WAD is 100*WAD
      _id = surplusAuctionHouse.startAuction({
        _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
        _initialBid: 0
      });

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