search

sherlock-audit / 2024-03-nouns-dao-2-judging

1 stars 0 forks source link

Krace - The auction duration is significantly shorter than expected when the contract is paused #9

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

Krace

medium

The auction duration is significantly shorter than expected when the contract is paused

Summary

When the NounsAuctionHouseV2 contract is paused, the current auction is still ongoing, but no one is able to continue bidding. In extreme cases, if a bidder places a bid and the contract is then paused until the current auction ends, that bidder may win the auction at an extremely low price, which is unfair to other bidders.

Vulnerability Detail

The current contract can settle auctions even when in a paused state, which could lead to potential issues.

    /**
     * @notice Settle the current auction.
     * @dev This function can only be called when the contract is paused.
     */
    function settleAuction() external override whenPaused {
        _settleAuction();
    }

Please consider the following scenario:

There are two bidders, bidder1 and bidder2, competing for the current nounId. Its value is approximately 10 USD, and the bidding time is 1 hour.

The bidder1 bids 1 USD first, followed by bidder2 bidding 2 USD. At this point, some emergencies occur, and the owner pauses the entire contract. However, the auction continues despite the contract suspension, leaving no opportunity for further bidding due to the contract's suspension.

In extreme cases, if the contract suspension persists until the end of the auction, which is 1 hour later, bidder2 can directly settle the auction and acquire the nounId at a price of 2 USD, while other bidders lose the opportunity to participate in bidding.

I believe this situation has affected the core functionality of the contract, and it is crucial to address this boundary scenario appropriately.

POC Add the test to nouns-monorepo/packages/nouns-contracts/test/foundry/NounsAuctionHouseV2.t.sol and run it with forge test --match-test test_settleAuction_After_pause --ffi

diff --git a/nouns-monorepo/packages/nouns-contracts/test/foundry/NounsAuctionHouseV2.t.sol b/nouns-monorepo/packages/nouns-contracts/test/foundry/NounsAuctionHouseV2.t.sol
index 9eff37b..2be619d 100644
--- a/nouns-monorepo/packages/nouns-contracts/test/foundry/NounsAuctionHouseV2.t.sol
+++ b/nouns-monorepo/packages/nouns-contracts/test/foundry/NounsAuctionHouseV2.t.sol
@@ -103,6 +103,34 @@ contract NounsAuctionHouseV2Test is NounsAuctionHouseV2TestBase {
         auction.createBid{ value: 1.49 ether }(nounId);
     }

+
+    function test_settleAuction_After_pause() public {
+        //There are two bidders : bidder1 and bidder2
+        uint256 nounId = auction.auction().nounId;
+        address bidder1 = address(0x4444);
+        address bidder2 = address(0x5555);
+
+        vm.deal(bidder1, 1.1 ether);
+        vm.prank(bidder1);
+        auction.createBid{ value: 1.1 ether }(nounId);
+        // The bidder2 wins the auction now
+        vm.deal(bidder2, 2.2 ether);
+        vm.prank(bidder2);
+        auction.createBid{ value: 2.2 ether }(nounId);
+
+        // The auction is paused
+        vm.prank(owner);
+        auction.pause();
+        // Time goes on
+        uint40 endTime = auction.auction().endTime;
+        vm.warp(endTime + 1);
+
+        // The bidder2 could settle Auction even if the whole auction actually only last for a
+        // very small period.
+        auction.settleAuction();
+        assertEq(auction.nouns().ownerOf(nounId), bidder2);
+    }
+
     function test_createBid_refundsPreviousBidder() public {
         uint256 nounId = auction.auction().nounId;
         address bidder1 = address(0x4444);

Impact

Bidders may successfully bid at prices significantly lower than the average, thus profiting from the total proceeds.

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/8f6879efaf831eb7fc9d4a4ad2b62b5334220d87/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV2.sol#L111-L113

Tool used

Foundry

Recommendation

It is advisable to cancel the current auction when the contract is paused.

sherlock-admin4 commented 4 months ago

2 comment(s) were left on this issue during the judging contest.

WangAudit commented:

I believe it is intended behaviour; settleAuction can be called only when the contract is paused and new auction is not started; therefore; I believe this function is specifically made to settle auctions when paused

takarez commented:

this is invalid according to sherlock rules.

eladmallel commented 4 months ago

By design indeed.