Revolution Protocol - alexbabits's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L346-#L355 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L287-#L291

Vulnerability details

Impact

Owner can make an auction fail by increasing the reserve price to an arbitrarily high number so the auction ends, but the verb gets burned and the top bidder gets refunded.

Imagine the owner doesn't want anyone to win the current auction. (They don't want the current top bidder to win, or are otherwise dissatisfied with the current bid amount). The owner can set the reserve price to 10,000 ETH during the auction even if people have started bidding. This causes the auction to end and can go to settlement, but the top bidder just gets refunded because the Auction House has less funds than the high reserve price that was set. So the top bidder gets refunded their bid and the verb gets burned.

Proof of Concept

Run with pnpm run test --match-path ./test/auction/AuctionSettling.t.sol --match-test testOwnerDisallowsAnyWinner

    function testOwnerDisallowsAnyWinner() public {

        //create art piece
        uint256 verbId = createArtPiece(
            "Art Piece",
            "A new art piece",
            ICultureIndex.MediaType.IMAGE,
            "ipfs://image",
            "",
            "",
            address(0x1),
            10_000
        );

        //set bps
        uint256 creatorRate = auction.creatorRateBps();
        uint256 entropyRate = auction.entropyRateBps();
        
        auction.unpause(); //start auction

        uint256 bidAmountOne = 2 ether;
        uint256 bidAmountTwo = 3 ether;
        vm.deal(address(0xAAAAAA), bidAmountOne);
        vm.deal(address(0xBBBBBB), bidAmountTwo);

        // 0xAAAAAA bids 2 ETH
        vm.startPrank(address(0xAAAAAA));
        auction.createBid{ value: bidAmountOne }(verbId, address(0xAAAAAA));
        vm.stopPrank();

        vm.startPrank(address(dao));
        auction.setReservePrice(10000 ether); // ⚠️Set very high during auction so nobody else can bid⚠️.
        vm.stopPrank();
        assertEq(auction.reservePrice(), 10000 ether);

        // 0xBBBBBB bids 3 ETH (REVERTS)
        vm.startPrank(address(0xBBBBBB));
        vm.expectRevert();
        auction.createBid{ value: bidAmountTwo }(verbId, address(0xBBBBBB));
        vm.stopPrank();

        vm.warp(block.timestamp + auction.duration() + 1); // time after auction ends
        auction.settleCurrentAndCreateNewAuction(); // end auction

        // Nobody "won" the auction because reserve price not met.
        // So 0xAAAAAA, even though they are top bidder, gets refunded and the verb get burned.
        assertEq(address(0xAAAAAA).balance, 2 ether);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider having AuctionHouse::setReservePrice() only be callable by the owner when there is no auction ongoing with the whenPaused modifier check.

-    function setReservePrice(uint256 _reservePrice) external override onlyOwner {
+    function setReservePrice(uint256 _reservePrice) external override onlyOwner whenPaused {
        reservePrice = _reservePrice;
        emit AuctionReservePriceUpdated(_reservePrice);
    }

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L361 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L297-#L301

Vulnerability details

Impact

Owner can drastically increase minBidIncrementPercentage increment during ongoing auction. Owner can grief an ongoing auction by calling AuctionHouse::setMinBidIncrementPercentage() and setting the minimum bid increment percentage to a higher number than the default setting of 5%, up to 255, or 255%.

Proof of Concept

Run with: pnpm run test --match-path ./test/auction/AuctionSettling.t.sol --match-test testOwnerDrasticallyIncreaseBidIncrementDuringAuction

    function testOwnerDrasticallyIncreaseBidIncrementDuringAuction() public {

        //create art piece
        uint256 verbId = createArtPiece(
            "Art Piece",
            "A new art piece",
            ICultureIndex.MediaType.IMAGE,
            "ipfs://image",
            "",
            "",
            address(0x1),
            10_000
        );

        //set bps
        uint256 creatorRate = auction.creatorRateBps();
        uint256 entropyRate = auction.entropyRateBps();

        auction.unpause(); //start auction

        uint256 bidAmountOne = 2 ether;
        uint256 bidAmountTwo = 3 ether;
        vm.deal(address(0xAAAAAA), bidAmountOne);
        vm.deal(address(0xBBBBBB), bidAmountTwo);

        // 0xAAAAAA bids 2 ETH
        vm.startPrank(address(0xAAAAAA));
        auction.createBid{ value: bidAmountOne }(verbId, address(0xAAAAAA));
        vm.stopPrank();

        // Initially minimum bid increment percentage is 5%.
        vm.startPrank(address(dao));
        auction.setMinBidIncrementPercentage(255); // ⚠️255% minimum bid increment increase during auction⚠️
        vm.stopPrank();

        // 0xBBBBBB bids 3 ETH (REVERTS) even though it's a 50% increase from previous bid amount.
        vm.startPrank(address(0xBBBBBB));
        vm.expectRevert();
        auction.createBid{ value: bidAmountTwo }(verbId, address(0xBBBBBB));
        vm.stopPrank();

        vm.warp(block.timestamp + auction.duration() + 1); // time after auction ends
        auction.settleCurrentAndCreateNewAuction(); //end auction
        
        // ⚠️ 0xAAAAAA wins auction due to shenanigans ⚠️
        assertEq(address(0xAAAAAA).balance, 0 ether);
        assertEq(erc721Token.ownerOf(verbId), address(0xAAAAAA), "Verb given to winner");
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider having AuctionHouse::setMinBidIncrementPercentage() only be callable by the owner when there is no auction ongoing with the whenPaused modifier check.

-   function setMinBidIncrementPercentage(uint8 _minBidIncrementPercentage) external override onlyOwner {
+   function setMinBidIncrementPercentage(uint8 _minBidIncrementPercentage) external override onlyOwner whenPaused {
        minBidIncrementPercentage = _minBidIncrementPercentage;
        emit AuctionMinBidIncrementPercentageUpdated(_minBidIncrementPercentage);
    }

Assessed type

Timing