safeTransferFrom in AuctionDemo.claimAuction can be abused to Reentrancy attacks

[c4-submissions]

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

[c4-sponsor]

a2rocket (sponsor) confirmed

[c4-judge]

alex-ppg marked the issue as duplicate of #1547

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)