Current mitigation allows placeBid() and settleAuction() racing conditions, previous highest bidder can prevent others to bid

[c4-bot-4]

Lines of code

https://github.com/pixeldaogg/florida-contracts/blob/7212bfbe9f78ca6eabb5eec86e24d754feb47f15/src/lib/AuctionLoanLiquidator.sol#L247 https://github.com/pixeldaogg/florida-contracts/blob/7212bfbe9f78ca6eabb5eec86e24d754feb47f15/src/lib/AuctionLoanLiquidator.sol#L283

Vulnerability details

Impacts

Racing conditions between settleAuction and placeBid might allow previous highest bidder to prevent others to bid.

Original Issues/Mitigation

C4 Issue: M-12 Pool.getMinTimeBetweenWithdrawalQueues current calculations may not be sufficient

Original vulnerabilities: The queues in Pool.sol are managed as a circular array. The oldest queue will be overwritten when a new queue is deployed once the max queue number is reached.

The issue raises a potential edge case where an Auction with MAX_AUCTION_DURATION = 7 days for a loan with a max duration (IPoolOfferHandler(_offerHandler).getMaxDuration()) might cause the queue where the loan is initiated to be deleted before the auction settles.

Original impacts: The impact is when the loan’s liquidation is settled with extended bidding time (past MAX_AUCTION_DURATION), the queue where the loanId initiated might have been overwritten already.

Mitigation: Fix https://github.com/pixeldaogg/florida-contracts/pull/384/files

//src/lib/AuctionLoanLiquidator.sol
...
    /// @notice Max extension for an auction.
|>  uint96 public immutable getMaxExtension;
...
    function placeBid(address _nftAddress, uint256 _tokenId, Auction memory _auction, uint256 _bid)
        external
        nonReentrant
        returns (Auction memory)
    {
...
        uint96 maxExtension = expiration + getMaxExtension;
        uint96 minWithMarginMaxExtension = withMargin > maxExtension ? maxExtension : withMargin;
        /// @dev max (min(withMargin, maxExtension), expiration)
        uint96 max = minWithMarginMaxExtension > expiration ? minWithMarginMaxExtension : expiration;
|>      if (max < currentTime && currentHighestBid > 0) {
            revert AuctionOverError(max);
        }
...
    function settleAuction(Auction calldata _auction, IMultiSourceLoan.Loan calldata _loan) external nonReentrant {
...
        uint96 maxExtension = expiration + getMaxExtension;
|>      if ((withMargin > currentTime && currentTime < maxExtension) || (currentTime < expiration)) {
            uint96 minWithMarginMaxExtension = withMargin > maxExtension ? maxExtension : withMargin;
            uint96 max = minWithMarginMaxExtension > expiration ? minWithMarginMaxExtension : expiration;
            revert AuctionNotOverError(max);
        }
...

The mitigation implemented is to limit the extended auction to a manageable time window (maxExtension). I consider this mitigated the impact of said edge case raised in the issue, because (1) it limits the extended period to a cap; (2) The default auction duration is an onlyOwner set value currently only 3 days by default(uint48 internal _liquidationAuctionDuration = 3 days;)

New Issue

This introduces a new issue: placeBid() and settleAuction() now both allow the same max timestamp, which encourages a racing condition.

For example, suppose in placeBid(), the max == expiration. if (max < currentTime && currentHighestBid > 0) {//revert will allow it when block.timestamp == max (expriation).

However in settleAuction(), max == expiration. if ((withMargin > currentTime && currentTime < maxExtension) || (currentTime < expiration)) {//revert will also allow it when block.timestamp == expiration.

The attack scenario:

1. UserA placeBid() before max (expiration) timestamp.
2. UserB placeBid() at expiration timestamp to outBid UserA.
3. UserA sees UserB's tx and front-run UserB with settleAuction(). UserA's settleAuction() tx settle first at expiration timestamp. UserA successfully prevents UserB's outbid.

Recommendations:

In placeBid(), change the check condition into if (max <= currentTime && currentHighestBid > 0) {//revert

Assessed type

Other

[0xend]

Agree it's an issue. Not sure it's a medium (why would User B be wait till the very last second for this to happen?)

[alex-ppg]

The Warden describes an edge case whereby the placeBid and settleAuction functions can be executed at the same timestamp, however, the described misbehavior is of minimal impact. The attack can only be carried out on a bid that would be processed at the last possible second which is a highly unlikely scenario, and the impact is once again constrained to a single highly abnormal bid being denied.

I agree with the Sponsor's assessment that this merits a QA rating, and it does not relate to the mitigation of M-12 which is an entirely different issue.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Overinflated severity

[c4-judge]

alex-ppg marked the issue as primary issue

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)