NextGen - Neon2835's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L57-L61 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L124-L130 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L104-L120

Vulnerability details

Impact

In the auctionDemo contract, anyone can participate in bidding for NFTs, and there is no minimum threshold for each bid in the contract. We assume that the current market price of NFTs is around 0.1ETH. Malicious bidders include two bids in a transaction at the beginning of the auction. The first bid is very small, such as 1 wei ETH, and the second bid is very large, such as 100 ETH making themselves the highest bidder. When the auction reaches its end time, the malicious bidder cancels their second bid and instead makes their first bid the current highest bid. Ultimately, the malicious attacker can call the claimAuction function to obtain NFT at a price of 1wei.

Proof of Concept

The key to the attack is that a transaction includes a very small quotation, followed by a very large quotation. In the participateToAuction function of the auctionDemo contract, it is specified that any user participating in the auction must bid greater than the current maximum price. The code is as follows:

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);
}

The attacker successfully occupies the highest price with a high quote. He needs to calculate the time and call the cancelBid function to cancel his highest quote and retrieve his ETH in the nearest block at the end of the auction.

function cancelBid(uint256 _tokenid, uint256 index) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
    auctionInfoData[_tokenid][index].status = false;
    (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
    emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
}

After reaching the auction end time, the attacker can smoothly call the claimAuction function to bid for NFT at a price of 1wei.

In fact, if the timestamp at the end of the auction is exactly equal to the time of a certain block, the attacker can even perform two actions simultaneously in that block: canceling the second quote while calling claimAuction to bid for NFT at a price of 1wei.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    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);
    address highestBidder = returnHighestBidder(_tokenid);
    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
            emit ClaimAuction(owner(), _tokenid, success, highestBid);
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

Tools Used

Foundry And VsCode

Recommended Mitigation Steps

In the participateToAuction function, remove the restriction that must be greater than the maximum price to participate in the auction. Everyone can quote at a price they think is reasonable. The person with the highest quote after the auction end time wins the NFT

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L104-L120 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L124-L130

Vulnerability details

Impact

In the auctionDemo contract, when the end time of the auction is exactly equal to the timestamp of a certain block, the winner of the auction retrieves the NFT by calling the claimAuction function first, and then immediately calling the cancelBid function again to retrieve the ETH he has already paid. At the same time, any other user who has participated in the bidding can make another withdrawal by calling the cancelBid function within the same block. This means that two refunds can be withdrawn, and the excess withdrawal takes away ETH that does not belong to them.

Proof of Concept

The reason for the vulnerability is that in the claimAuction function, the user's bidding status is not set to false Let's assume that the end time of a certain auction now exactly equals block.timestamp

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    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);
    address highestBidder = returnHighestBidder(_tokenid);
    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
            emit ClaimAuction(owner(), _tokenid, success, highestBid);
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

First refund: The winner called the claimAuction function to obtain the NFT, and the remaining users also received their ETH refunds. Second refund: As long as the user calls the cancelBid function again, they will receive a second refund. Let's take a look at the code:

function cancelBid(uint256 _tokenid, uint256 index) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
    auctionInfoData[_tokenid][index].status = false;
    (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
    emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
}

Note that both statements require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended"); and require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true); can be bypassed at this time, and the auctionDemo contract may hold ETH balances from multiple auctions at the same time. At this point, the second withdrawal will be successful.

Tools Used

Foundry And VsCode

Recommended Mitigation Steps

Optimization 1: Optimize the claimAuction function by adding auctionInfoData[_tokenid][i].status = false; Statement toto synchronize user status changes

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    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);
    address highestBidder = returnHighestBidder(_tokenid);
    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
+           auctionInfoData[_tokenid][i].status = false; 
            emit ClaimAuction(owner(), _tokenid, success, highestBid);
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
+           auctionInfoData[_tokenid][i].status = false; 
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

Optimization 2: Optimize the cancelBid function by changing the <= judgment condition to <

function cancelBid(uint256 _tokenid, uint256 index) public {
-   require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+   require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");
    require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
    auctionInfoData[_tokenid][index].status = false;
    (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
    emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
}

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

In the auctionDemo contract, the claimAuction function can be called by the winner or the administrator to transfer the NFT to the winner and return the ETH to the remaining users who have not yet cancelled the auction. If the winner's address is a contract, the contract can perform a malicious rollback when receiving the NFT, locking many people's ETH in the contract and making it impossible to withdraw. Malicious users can extort money through this kind of behavior.

Proof of Concept

Let's take a deep dive into the claimAuction function:

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    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);
    address highestBidder = returnHighestBidder(_tokenid);
    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
            emit ClaimAuction(owner(), _tokenid, success, highestBid);
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

Note that if the statement IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid); occurs revert, the entire transaction will also fail to complete. Continuing to dive deeper into the safeTransferFrom function, you will find the following code:

function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual {
    _transfer(from, to, tokenId);
    require(_checkOnERC721Received(from, to, tokenId, data), "ERC721: transfer to non ERC721Receiver implementer");
}

If a malicious contract is maliciously revert in it's onERC721Received callback function, the attack can be successfully completed.

Tools Used

Foundry And VsCode

Recommended Mitigation Steps

Use try catch statements to optimize NFT safeTransferFrom behavior

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The auctionDemo contract's participateToAuction function does not provide good condition verification.

When the auction ends, users can still participate. If a user calls the participateToAuction function at the auction's end timestamp, the ETH he provided for the bid may be permanently locked in the contract because the NFT may have already been claimed by the winner.

Proof of Concept

Let's assume that the timestamp of the end of the auction is exactly equal to the timestamp of a block.

Alice and Bob both want to win the NFT, and they broadcast their transactions at the same time within the last 12 seconds:

1. Assume that Alice has bid 1 ETH and is currently the winner of the entire auction. She broadcasts a transaction calling the claimAuction function to successfully claim the NFT.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    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);
    address highestBidder = returnHighestBidder(_tokenid);
    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
            emit ClaimAuction(owner(), _tokenid, success, highestBid);
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

2. Bob's transaction follows closely, and he broadcasts a transaction offering 2 ETH to call the participateToAuction function to participate in the bidding.

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);
}

3. The above transactions are completed simultaneously in the last block. At this point, Bob's 2 ETH will be locked in the contract forever!

4. Because the NFT has been taken by Alice, so Bob's call to the claimAuction function will not succeed. And the timestamp of the next block will be greater than the auction end timestamp, so Bob cannot call the cancelBid function to take his ETH back .

Tools Used

Foundry And VsCode

Recommended Mitigation Steps

optimize the participateToAuction function

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

Assessed type

Invalid Validation