code-423n4 / 2024-04-gondi-findings

0 stars 0 forks source link

Racing condition between settleAuction and placeBid might allow previous highest bidder to prevent others to bid #34

Closed c4-bot-3 closed 7 months ago

c4-bot-3 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/AuctionLoanLiquidator.sol#L238 https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/AuctionLoanLiquidator.sol#L273

Vulnerability details

Impact

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

Proof of Concept

In AuctionLoanLiquidator.sol, placeBid and settleAuction windows overlap. Both are allowed when currentTime == max (withMargin or expiration).

    function placeBid(address _nftAddress, uint256 _tokenId, Auction memory _auction, uint256 _bid)
...
        uint96 max = withMargin > expiration ? withMargin : expiration;
|>        if (max < currentTime && currentHighestBid > 0) {
            revert AuctionOverError(max);
        }
...

(https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/AuctionLoanLiquidator.sol#L237-L239)

    function settleAuction(Auction calldata _auction, IMultiSourceLoan.Loan calldata _loan) external nonReentrant {
...
        uint96 expiration = _auction.startTime + _auction.duration;
        uint96 withMargin = _auction.lastBidTime + _MIN_NO_ACTION_MARGIN;
|>        if ((withMargin > currentTime) || (currentTime < expiration)) {
            uint96 max = withMargin > expiration ? withMargin : expiration;
            revert AuctionNotOverError(max);

This is problematic because a user whose placeBid tx settles at max time might be reverted when another user front-runs with settleAuction at the same max time. This encourages a racing between settleAuction and placeBid, which allows previous highest bidder to prevent others to bid by front-running.

Tools Used

Manual

Recommended Mitigation Steps

In settleAuction, change the if control flow into if ((withMargin >= currentTime) || (currentTime <= expiration)) {//revert}

Assessed type

Other

c4-judge commented 7 months ago

0xA5DF marked the issue as duplicate of #6

c4-judge commented 7 months ago

0xA5DF marked the issue as satisfactory