NextGen - jasonxiale's results

Lines of code

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

Vulnerability details

Impact

calling safeTransferFrom in AuctionDemo.claimAuction contains a reentrancy vulnerability, by abusing safeTransferFrom, the malicious bidder can get the token without paying any ETH

Proof of Concept

To abusing the reentrancy vulnerability, 3 conditions need to be met:

1. safeTransferFrom has a callback
2. AuctionDemo.claimAuction doesn't have reentrancy protection, and can be called when block.timestamp >= minter.getAuctionEndTime(_tokenid), please note here the comparison is >=
3. AuctionDemo.cancelBid doesn't have reentrancy protection, and can be called when block.timestamp <= minter.getAuctionEndTime(_tokenid), please note here the comparison is <=

Suppose Alice depolys a contract to participate in the auction, and the contract is the highest bidder, and she calls AuctionDemo.claimAuction when (block.timestamp == minter.getAuctionEndTime(_tokenid)， the function will pass require check at AuctionDemo.sol#L105, and then will arrive AuctionDemo.sol#L112, within safeTransferFrom, contract's onERC721Received will be called

104     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true); <<<--- we call the function when block.timestamp == minter.getAuctionEndTime(_tokenid)
106         auctionClaim[_tokenid] = true;
107         uint256 highestBid = returnHighestBid(_tokenid);
108         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109         address highestBidder = returnHighestBidder(_tokenid);
110         for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111             if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
112                 IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid); <<< --- Here will call highestBidder.onERC721Received
113                 (bool success, ) = payable(owner()).call{value: highestBid}("");
114                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
115             } else if (auctionInfoData[_tokenid][i].status == true) {
116                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
117                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
118             } else {}
119         }
120     }

Within the onERC721Received function, the contract will call AuctionDemo.cancelBid or AuctionDemo.cancelAllBids. In AuctionDemo.cancelBid, the require will be passed because we're calling when block.timestamp == minter.getAuctionEndTime(_tokenid)

124     function cancelBid(uint256 _tokenid, uint256 index) public {
125         require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended"); <<<--- we're calling when block.timestamp == minter.getAuctionEndTime(_tokenid)
126         require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
127         auctionInfoData[_tokenid][index].status = false;
128         (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
129         emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
130     }

after calling AuctionDemo.cancelBid, Alice gets her ETH back, and as AuctionDemo.claimAuction continue, she also gets the auction token.

Tools Used

VIM

Recommended Mitigation Steps

add reentrancy protection

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

Calling payable().call in AuctionDemo.claimAuction contains a reentrancy vulnerability, by abusing it, the malicious bidder can get the token without paying any ETH. Please note the issue can be exploited other non-highest bidders as the following 3 conditions are met.

Proof of Concept

To abusing the reentrancy vulnerability, 3 conditions need to be met:

1. payable().call has a callback
2. AuctionDemo.claimAuction doesn't have reentrancy protection, and can be called when block.timestamp >= minter.getAuctionEndTime(_tokenid), please note here the comparison is >=
3. AuctionDemo.cancelBid doesn't have reentrancy protection, and can be called when block.timestamp <= minter.getAuctionEndTime(_tokenid), please note here the comparison is <=

Suppose Alice is the highest bidder, and she calls AuctionDemo.claimAuction when (block.timestamp == minter.getAuctionEndTime(_tokenid)， the function will pass require check at AuctionDemo.sol#L105, and then will arrive AuctionDemo.sol#L116 to send the ETHs back to other bidders. Since payable().call is used here, if the bidder is a contract and contains a fallback function, the fallback function will be called.

104     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true); <<<--- we call the function when block.timestamp == minter.getAuctionEndTime(_tokenid)
106         auctionClaim[_tokenid] = true;
107         uint256 highestBid = returnHighestBid(_tokenid);
108         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109         address highestBidder = returnHighestBidder(_tokenid);
110         for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111             if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
112                 IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
113                 (bool success, ) = payable(owner()).call{value: highestBid}("");
114                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
115             } else if (auctionInfoData[_tokenid][i].status == true) {
116                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}(""); <<< --- Here will call bidder's fallback function if the bidder has one
117                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
118             } else {}
119         }
120     }

Within the fallback function, the contract will call AuctionDemo.cancelBid or AuctionDemo.cancelAllBids. In AuctionDemo.cancelBid, the require will be passed because we're calling when block.timestamp == minter.getAuctionEndTime(_tokenid)

124     function cancelBid(uint256 _tokenid, uint256 index) public {
125         require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended"); <<<--- we're calling when block.timestamp == minter.getAuctionEndTime(_tokenid)
126         require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
127         auctionInfoData[_tokenid][index].status = false;
128         (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
129         emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
130     }

In such way, a malicious bidder can get double ETH of his bids.

Tools Used

Recommended Mitigation Steps

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

A malicious bidder, essentially the validator/MEVer who has the ability to slip in transaction can back-run AuctionDemo.claimAuction to steal ETH.

Proof of Concept

To exploit this issue, it requires that AuctionDemo.claimAuction must be called when block.timestamp == minter.getAuctionEndTime(_tokenid) In such case, when the highest bidder claim his auction, all other bidders' ETH will be sent back by AuctionDemo.sol#L116. But in AuctionDemo.claimAuction the function doesn't set auctionInfoData[_tokenid][index].status = false; as AuctionDemo.cancelBid does.

    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; <<<------- auctionInfoData.status is set to false, so this funciton can't be called again.
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

Because of lack of setting auctionInfoData[_tokenid][index].status, it opens door to malicious bidder, he can back-run AuctionDemo.claimAuction by calling AuctionDemo.cancelBid or AuctionDemo.cancelAllBids in the same block, in such case, the require won't revert because of the tx is still under the condition of block.timestamp == minter.getAuctionEndTime(_tokenid) By doing so, the malicious bidder can steal ETH

Tools Used

VIM

Recommended Mitigation Steps

diff --git a/hardhat/smart-contracts/AuctionDemo.sol b/hardhat/smart-contracts/AuctionDemo.sol
index 95533fb..74cf250 100644
--- a/hardhat/smart-contracts/AuctionDemo.sol
+++ b/hardhat/smart-contracts/AuctionDemo.sol
@@ -113,6 +113,7 @@ contract auctionDemo is Ownable {
                 (bool success, ) = payable(owner()).call{value: highestBid}("");
                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
             } else if (auctionInfoData[_tokenid][i].status == true) {
+                auctionInfoData[_tokenid][i].status = false;
                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
             } else {}
@@ -148,4 +149,4 @@ contract auctionDemo is Ownable {
         return auctionInfoData[_tokenid];
     }
 
-}
\ No newline at end of file
+}

Assessed type

MEV