NextGen - Madalad's results

Lines of code

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

Vulnerability details

Impact

If the end time of an auction happens to be equal to the timestamp of a mined block, the winner of that auction is able to call claimAuction to receive their NFT, and then call cancelBid to cancel their winning bid, essentially getting the NFT for free and stealing funds from the owner of the AuctionDemo contract. The attacker can place a bid that is far greater than the value of the NFT, ensuring that they are not outbid, since they know their ETH will be refunded anyway.

This protocol is to be deployed on Ethereum mainnet only. Since the move to proof of stake, the time in between blocks is consistently 12 seconds. This means that the probability for the precondition of this attack occurring by chance is 1/12. However, due to the fact that each block time is exactly 12 seconds, a malicious user can easily calculate whether the end time for an auction will be equal to a future blocks timestamp, and plan to perform this attack ahead of time.

In short, roughly one out of every 12 auctions is highly vulnerable to the NFT being stolen.

Proof of Concept

This attack is possible because when the end auction time is equal to block.timestamp, the AuctionDemo contract perceives the auction as both in progress, and finished at the same time. This is evident from the fact that each check on block.timestamp uses a weak inequality (>= or <=) instead of a strict one.

File: src\AuctionDemo.sol
103: 
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);

123: 
124:     function cancelBid(uint256 _tokenid, uint256 index) public {
125:         require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

133: 
134:     function cancelAllBids(uint256 _tokenid) public {
135:         require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

L105 L125 L135

Moreover, claimAuction does not update the status of each bid to false as it refunds losing bids or resolves the winning bid, meaning that when cancel is subsequently called the bid is treated as unresolved. This allows the highest bidder to call claimAuction and then call cancelBid afterwards but in the same block. Since block.timestamp is equal to the returned value of getAuctionEndTime, the require checks in both functions will pass.

The below PoC was made using foundry. Please see this gist for the full test file.

    function testCancelBidAfterClaim() public {
        // Setup
        address alice = makeAddr("alice");
        vm.deal(address(auctionDemo), 10 ether);
        vm.deal(alice, 1 ether);
        uint256 aliceBalanceBefore = address(alice).balance;

        // Create auction
        uint256 auctionEndTime = block.timestamp + 100;
        vm.startPrank(admin);
        minterContract.mintAndAuction(
            address(admin),
            "token data",
            0,
            1,
            auctionEndTime
        );
        coreContract.setApprovalForAll(address(auctionDemo), true);
        vm.stopPrank();
        uint256 tokenId = 10000000000;
        assertTrue(minterContract.getAuctionStatus(tokenId));

        // alice bids
        uint256 bidAmount = 1 ether;
        vm.startPrank(alice);
        auctionDemo.participateToAuction{ value: bidAmount }(tokenId);
        assertEq(address(alice).balance, aliceBalanceBefore - bidAmount);

        // Auction ends, alice is the highest bidder
        vm.warp(auctionEndTime);
        assertEq(auctionDemo.returnHighestBidder(tokenId), alice);
        
        // Alice claims NFT
        vm.warp(auctionEndTime);
        auctionDemo.claimAuction(tokenId);

        // Alice unfairly cancels her bids to reclaim her ETH
        vm.warp(auctionEndTime);
        auctionDemo.cancelAllBids(tokenId);

        // Alice received the NFT and her ETH back
        uint256 aliceBalanceAfter = address(alice).balance;
        assertEq(coreContract.ownerOf(tokenId), alice);
        assertEq(aliceBalanceBefore, aliceBalanceAfter);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Alter the require statement in claimAuction to use a strict inequality. This ensures that cancelBid and cancelAllBids can never be called after claimAuction.

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
-       require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
+       require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

A malicious user can ensure that they win the auction for an arbitrarily low amount of ETH, preventing others from outbidding them for the duration of the auction. This breaks the auction system, results in a loss of funds to the owner of AuctionDemo (the address that receives the ETH from the highest bid) and provides a poor user experience for honest bidders.

Proof of Concept

AuctionDemo allows NextGenCore NFTs to be auctioned to the highest bidder. Users may place a bid using participateInAuction() and can cancel bids using cancelBid() or cancelAllBids() as long as the auction is ongoing. participateInAuction() prevents users from placing bids that are lower than the current highest bid.

File: src\AuctionDemo.sol

57:     function participateToAuction(uint256 _tokenid) public payable {
            // @audit reverts if msg.value is not greater than current highest bid
58:         require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
59:         auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
60:         auctionInfoData[_tokenid].push(newBid);
61:     }

A malicious user can use this to their advantage. Firstly, they place a small bid at the beginning of the auction, say 1 wei, and then they place a very large bid straight afterwards. Assuming that no one else would want to purchase the NFT at that price, no more bids are placed during the auction. Just as the auction is about to end, the bidder can cancel their second large bid, meaning that their measly 1 wei bid becomes the highest bid and they win the NFT practically for free.

A note on likelihood: if the auction end time happens to fall exactly on the block.timestamp of a future block, then this attack is trivial, as the attacker would be able to cancel their high bid and claimAuction to claim their NFT in the same transaction (this is because the timestamp checks in each of AuctionDemo's functions use weak inequalities, <= or >=). The chance of this occurring for any given auction is 1/12 (12 seconds is the time between blocks on Ethereum mainnet). If this does not happen to be the case, then the attack is still possible if block stuffing ([1] [2]) is used, or if the attacker is/has bribed a validator. Moreover, even if the attacker has to cancel their bid in the previous block and leave a window for legitimate bidders to outbid his initial 1 wei bid, that would still mean the auction process is DoSed for the entire duration (except the last few seconds), which itself is a severe violation of protocol functionality.

Recommended Mitigation Steps

Allow users to place bids even if they do not outbid the current highest bidder. As ETH is refunded at the end of the auction anyway, this does not lead to any adverse effects. To maintain a clean user experience, ensure that users are made aware whether their bid will be the current highest or not on the frontend (smart contracts that interact with )

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If the winner of an auction is a smart contract that does not implement onERC721Received, or if it is implemented but leads to a revert (which could be done unintentionally or maliciously) then the NFT, as well as the funds of all active bids will be trapped in the contract indefinitely.

Proof of Concept

AuctionDemo#claimAuction uses ERC721's safeTransferFrom to transfer the auctioned NFT from the current owner to the winning bidder. This function checks whether the receiver is a smart contract and if it is, attempts to call onERC721Received.

File: src\AuctionDemo.sol

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);
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) {
                     // @audit line below may revert if highestBidder does not implement onERC721Received
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}("");
117:                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
118:             } else {}
119:         }
120:     }

This scenario would lead to a complete DoS of claimAuction. Since it is the only function that is capable of sending the NFT and the ETH of the bidders from the contract, those funds would all be trapped in the contract.

• there is no way to avoid executing the safeTransferFrom call
• no new bids can be placed to replace highestBidder with a different address that is capable of receiving the NFT
• losing bidders cannot call cancelBid or cancelAllBids to retrieve their ETH because they revert if the auction is completed
• admins have no emergency function to retrieve the NFT or the ETH.

Recommended Mitigation Steps

Use transfer instead of safeTransfer. Although an auction winner that does not implement onERC721Received may not be able to access their NFT, they wouldn't have been able to receive it anyway if safeTransfer was used, and this way losing bidders still get their ETH back.

    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);
+               IERC721(gencore).transferFrom(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 {}
        }

Assessed type

ERC721

QA Report

setCollectionCosts and setCollectionPhases can be used to change key values during the minting phase

In MinterContract, function admins may call setCollectionCosts to set values used to determine the cost of minting at a particular timestamp, and setCollectionPhases to set the timestamps at which the allowlist/public minting phase begin and end.

These function must have been before users can call mint, however there is nothing stopping these setters from being called multiple times afterward during minting. Such functionality is not desirable as it could be used deceive and/or exploit users. Ensure that they are only callable once per collection, or that they cannot be called after allowlistStartTime.

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L157-L177

XRandoms#getWord does not work properly

XRandoms#getWord chooses one of 100 words depending on the random id number passed as an argument, however the logic is incorrect. Since it is only called by the randomWord function, we know that % 100 will have been applied to id, meaning it is between 0 and 99 inclusive, and never 100. This means that the word at index 0 ("Acai") has a 2/100 chance of being chosen, while the word at index 99 ("Watermelon") will never be chosen.

Change the function to always return wordsList[id] to ensure each word has an equal chance of being chosen.

File: smart-contracts\XRandoms.sol

    function getWord(uint256 id) private pure returns (string memory) {
        
        // array storing the words list
        string[100] memory wordsList = ["Acai", "Ackee", "Apple", "Apricot", "Avocado", "Babaco", "Banana", "Bilberry", "Blackberry", "Blackcurrant", "Blood Orange", 
        "Blueberry", "Boysenberry", "Breadfruit", "Brush Cherry", "Canary Melon", "Cantaloupe", "Carambola", "Casaba Melon", "Cherimoya", "Cherry", "Clementine", 
        "Cloudberry", "Coconut", "Cranberry", "Crenshaw Melon", "Cucumber", "Currant", "Curry Berry", "Custard Apple", "Damson Plum", "Date", "Dragonfruit", "Durian", 
        "Eggplant", "Elderberry", "Feijoa", "Finger Lime", "Fig", "Gooseberry", "Grapes", "Grapefruit", "Guava", "Honeydew Melon", "Huckleberry", "Italian Prune Plum", 
        "Jackfruit", "Java Plum", "Jujube", "Kaffir Lime", "Kiwi", "Kumquat", "Lemon", "Lime", "Loganberry", "Longan", "Loquat", "Lychee", "Mammee", "Mandarin", "Mango", 
        "Mangosteen", "Mulberry", "Nance", "Nectarine", "Noni", "Olive", "Orange", "Papaya", "Passion fruit", "Pawpaw", "Peach", "Pear", "Persimmon", "Pineapple", 
        "Plantain", "Plum", "Pomegranate", "Pomelo", "Prickly Pear", "Pulasan", "Quine", "Rambutan", "Raspberries", "Rhubarb", "Rose Apple", "Sapodilla", "Satsuma", 
        "Soursop", "Star Apple", "Star Fruit", "Strawberry", "Sugar Apple", "Tamarillo", "Tamarind", "Tangelo", "Tangerine", "Ugli", "Velvet Apple", "Watermelon"];

        // returns a word based on index
-       if (id==0) {
-           return wordsList[id];
-       } else {
-           return wordsList[id - 1];
-       }

+       return wordsList[id];
    }

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/XRandoms.sol#L28-L32

Insecure source of randomness used

Achieving randomness by hashing block constants, as is the method used by RandomizerNXT, is insecure because the values can be predicted. Moreover, miners/validators who control the block have the ability to manipulate certain values which can be abused to make sure the random value is in their favour.

The Additional Context section of the contest README states that RandomizerNXT will be used over RandomizerRNG and RandomizerVRF when the project is deployed. Reconsider this choice if true verifiable randomness is desired.

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