Bad debt auctions can be DoSed forever

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L183-L183 https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L248 https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L190

Vulnerability details

Vulnerability Details

For function Shortfall::placeBid() in shortfall contract on L#183 and L#190 , the previous highest bidder’s funds stored in the shortfall contract has to be sent back to the bidder. This operation has to be successful before any new bid can be placed.

if (auction.highestBidder != address(0)) {
    -------
        erc20.safeTransfer(auction.highestBidder, previousBidAmount); 
    }else {

      if (auction.highestBidder != address(0)) {
          erc20.safeTransfer(auction.highestBidder, auction.marketDebt[auction.markets[i]]);
      }
      --------
  }

The same is required in Shortfall::closeAuction(). L247-L248 :

function closeAuction(address comptroller) external nonReentrant {
        --------
        uint256 transferredAmount = riskFund.transferReserveForAuction(comptroller, riskFundBidAmount);
        IERC20Upgradeable(convertibleBaseAsset).safeTransfer(auction.highestBidder, riskFundBidAmount);
        --------
}

In the situation that the asset which is auctioned is a token implementing pre or post transfer hooks e.g ERC-677, ERC-777, a malicious user can register a hook after bidding, which reverts all transactions sent to it’s account, thereby DoSing the shortfall contract, and no bid can be placed again to the contract, leaving the malicious user as the winner of the bid when it is closed.

The same situation is possible in a scenario where the underlying asset is a contract that implements blacklisting on transfer e.g USDC. Once the highest bidder is blacklisted, no new bids can be placed.

Impact

Tokens implementing hooks or blacklisting may DoS bad debt auctions. And as mentioned in other submission - “Bad debt bidders’ funds are locked forever when Shortfall address is changed during ongoing debt auction”, if the protocol decides to mitigate the risk by changing the Shortfall address, then the bidder’s fund will be locked forever in the smart contract.

Proof of Concept

Scenario one:

1. Market A underlying token (BetaToken) is an ERC777 or ERC677 in Pool Alpha.
2. View open auctions on Shortfall against pool Alpha’s bad debts
3. Make the lowest possible bid that makes you the new highest bidder,
4. Register your address against a tokensReceived hook on BetaToken contract, your new hook implementation should revert all transactions that attempt to update your balance through transfer on BetaToken.
5. Another user attempts to make a bid, and its bid is higher than yours. placeBid(address,uint256) must first send your asset back to you, but when it attempts that, the transaction is reverted thanks to the tokensReceived hook registered against your address on BetaToken.
6. Once the bid has reached its stale period, close the bid and take your rewards. You won the auction.

Scenario two:

1. Market A’s underlying asset is a token contract (DeltaToken) that blacklists address, which means no funds is allowed to be sent into the address or out when blacklisted.
2. Market A is in Pool Alpha.
3. User A makes a bid against pool Alpha’s bad debts on Shortfall.
4. User A gets blacklisted on DeltaToken for stealing funds from other users.
5. Henceforth no one can make another bid in pool Alpha because all attempts to transfer User A’s DeltaTokens it bidded with, will fail and be reverted.

Scenario three:

1. The convertibleBaseAsset is either a token contract implementing hooks or blacklisting
2. If convertibleBaseAsset token contract is ERC677 or ERC777 and User A implements the hooks to revert all transaction to its address as described above after bidding and being the highest bidder, the shortfall contract wont ever be able to close the pool auction anymore, because any attempt to send funds to the user A address on the convertibleBaseAsset Token contract will be reverted.
3. If The convertibleBaseAsset implements blacklisting, once the User A account is blacklisted on convertibleBaseAsset token contract after bidding and being the highest bidder on ShortFall contract. The auction for the Pool on Shortfall wont be able to ever be closed as any attempt will be reverted by convertibleBaseAsset token contract, thus Dosing the Shortfall contract.

Tools Used

Manual analysis

Recommended Mitigation Steps

Use the pull over push design pattern for funds transfer.

Assessed type

DoS

[c4-judge]

0xean marked the issue as unsatisfactory: Overinflated severity

[deliriusz]

@0xean , I would like you to reconsider this finding. I do understand that it's arguable to put this finding as HIGH. We based our decision on the fact that we believe that impact of this is critical, and likelihood is medium, which by standard way of measuring impact gives a high impact. However, I believe that completely invalidating it is too harsh and it deserves at least MED. For example this issue https://github.com/code-423n4/2023-05-venus-findings/issues/305 - is concerning the same situation and points to the same root case, and it's accepted as a valid MED. Could you please take a look at this issue once again, please?

[c4-judge]

0xean marked the issue as duplicate of #305

[0xean]

@deliriusz - please review the c4 documentation on severities. I honored your request, but don't believe my original judgement was too harsh in any way.

[c4-judge]

0xean changed the severity to 2 (Med Risk)