Lack of Reentrancy guard on auctionSurplus() function

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

auctionSurplus() function have call backs that can lead to reentrancy and manipulating the transfer of tokens. Malicious actor can manipulate token transfer by changing the _destination address to himself and get all the extra tokens.

Proof of Concept

auctionSurplus() calls the transferInternalCoins method of contract safeEngine which have _destination: extraSurplusReceiver, that can be used by malicious attacker for gaining all surplus funds.

  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) {
      _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));
    }

    // 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));
    }
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Add nonReentrant reentrancy guard modifier by OpenZeppeling on the auctionSurplus() function.

  function auctionSurplus() external nonReentrant  returns (uint256 _id) {
``` 

## Assessed type

Reentrancy

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #45

[c4-judge]

MiloTruck marked the issue as unsatisfactory: Insufficient proof