AuctionDemo.sol - Malicious bidder can block the execution of claimAuction()

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

At the end of the auction, the winner or admin can execute function claimAuction().
This function transfers NFT to the winner of the auction and iterates over auctionInfoData[_tokenid] to refund bids to other users who hadn't won the auction.

It's possible that auctionInfoData[_tokenid] may contain the address which will make claimAuction() to always revert. In other words, it won't be possible to call claimAuction(), thus the winner won't be able to get NFT and other users won't be able to get their non-winning bids refunded.

Proof of Concept

Whenever we bid, our address is being added to auctionInfoData[_tokenid]:

File: AuctionDemo.sol

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

Whenever auction ends, function claimAuction() iterates over auctionInfoData[_tokenid] and refunds bids to addresses which hadn't won the auction.

File: AuctionDemo.sol

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 {}

Function call() is being used to perform a refund: payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");.
This poses huge security risk, since we call addresses inserted by other users. In the following PoC, we will demonstrate how this can be exploited.
What we want to achieve, is trying to insert malicious contract address to auctionInfoData[_tokenid]. When function claimAuction() will perform call to this address, we want that call to force claimAuction() to revert.

According to 63/64 Rule, the callee can use the maximum of 63/64 gas available. This implies, that we should have at least 1/64 of our gas when call is done.

However, there are two attack scenarios which allow to exhaust the remaining gas and make the function claimAuction() to revert with out-of-gas error.

Scenario 1 - Using call() multiple of times

Provided code base performs multiple of calls in a single function, as we're performing call in a loop. Let's assume that every call() consumes the maximum possible amount of gas.
This implies, that after the first call(), we will remain with 1/64 gas.
After the 2nd call(), we will remain with 1/64 * 1/64 gas.
After the 3rd call(), we will remain with 1/64 * 1/64 * 1/64 gas.

Since blocks have a maximum limit of 30,000,000 gas units worth of transactions, we can assume that this is the max amount of gas we can spend for our transaction. This is, however, very unrealistic assumption, since basically not everyone can afford to pay for so much gas. In reality, the amount of gas used for the transaction will be much smaller. Nonetheless, we'll assume the max possible value.

According to EVM Codes, CALL uses at least 100 gas.
This suggests, that after 3rd call(), we will basically run out of gas and revert:

30 000 000 / 64 / 64 / 64 = 114

Scenario 2 - returndatacopy can exhaust remaining 1/64 gas

Even if it's not explicitly stated, solidity performs a returndatacopy for external calls. The described behavior of this issue can be found here: github.com/ethereum/solidity/issues/12306
This means, that even (bool success, ) = addr.call() will perform returndatacopy.
If the external contract uses 63/64 gas and returns very large data - it's possible to exhaust the remaining 1/64, thus caller might be forced to revert due to out-of-gas.

Let's examine how this can be used to exploit claimAuction().

PoC - Scenario 1

Let's consider a malicious smart contract which implements below receive() function:

receive() payable external {
	uint i;
	while (true) i++;
}

Preferably at the beginning of the auction (so the bid will still be small), mentioned contract performs participateToAuction() call.
The address of that contract is being inserted into auctionInfoData[_tokenid].
Contract should perform a few more participateToAuction() calls, so its address will be stored in auctionInfoData[_tokenid] multiple of times.

Now, auction ends and the winner calls claimAuction() function.

File: AuctionDemo.sol

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 {}

Function iterates over auctionInfoData[_tokenid] and performs the refund:

(bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");

The problem occurs, when it finally reaches the address of the malicious contract. Since malicious contract implements:

receive() payable external {
	uint i;
	while (true) i++;
}

the call to that contract will use 63/64 gas and claimAuction() will be left with 1/64 remaining gas.

However, please remember, that we had performed multiple of participateToAuction() calls from that address, thus another loop iteration will hit that address again.
After another call, we will be left with 1/64 * 1/64 remaining gas.
After another call - with 1/64 * 1/64 * 1/64 remaining gas
And so on - until there will be no gas enough to enter another call. Then, claimAuction() will revert with out-of-gas error.

To summarize:

1. Malicious contract (which implements infinite loop in its receive()) participates in the auction.
2. That contract sends multiple of participateToAuction() calls, thus its address is being added to auctionInfoData[_tokenid] many times.
3. When auction ends, function claimAuction() iterates over auctionInfoData[_tokenid] and refunds bids to the bidders.
4. Since auctionInfoData[_tokenid] contains multiple of contract's addresses which exhaust 63/64 on each call - claimAuction() will finally won't get enough gas to execute.
5. claimAuction() will revert due to out-of-gas error.
6. This scenario will be the same, every time we will try to call claimAuction(), which implies, it won't be possible to call this function.
7. Since it's not possible to call claimAuction(), winner won't be able to get NFT and get their bid back. Other users who bid, also won't be able to get their bid back.

PoC - Scenario 2

This is basically the same scenario, but the malicious contract implements a receive() function which exhaust 63/64 gas and returns the very large data. Since returndatacopy will exhaust remaining 1/64 gas, the call will immediately revert.

Malicious contract calls participateToAuction() (preferably at the beginning of the auction, so the bid is still small).
The address of the contract is being added to auctionInfoData[_tokenid].
When auction ends, the winner or admin calls claimAuction().
Function iterates over auctionInfoData[_tokenid] and performs the refund:

(bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");

The problem occurs, when it finally reaches the address of the malicious contract.
Malicious contract exhaust 63/64 gas and 1/64 remaining gas with returndatacopy. Function claimAuction() doesn't have enough gas to continue - thus it reverts with out-of-gas-error.
Since it's not possible to call claimAuction(), the winner cannot get NFT and noone is able to get a refund for their non-winning bids.

Tools Used

Manual code review

Recommended Mitigation Steps

claimAuction() should not iterate over auctionInfoData[_tokenid].
It's much better idea to divide that function into two smaller functions which would allow to refund/claim NFT to the caller.
This is a list of steps which should be taken:

• 1. Remove function claimAuction().
• 2. Implement function claimNFTReward() which will be called by the winner (or admin). This function should pay the auction price and transfer NFT.
• 3. Implement function refundBid(), this will be basically a copy&paste of cancelAllBids() function, but it needs to be executed when auction has ended.
     This function will perform call() only on msg.sender - thus even when some malicious contract will be added to auctionInfoData[_tokenid], it won't affect other users.
     Please make sure to verify that refundBid() won't allow to refund the bid which won the auction (otherwise, winner will be able to call claimNFTReward() and refundBid() afterwards).

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #1632

[c4-pre-sort]

141345 marked the issue as duplicate of #843

[c4-pre-sort]

141345 marked the issue as duplicate of #486

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1782

[c4-judge]

alex-ppg marked the issue as satisfactory