NextGen - seeques's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L193-L200 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L227-L232 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L213-L217 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L234-L237

Vulnerability details

Impact

It is possible to bypass max allowance restriction both for WL addresses and public ones. Since getting whitelisted usually is not that easy and it takes time and effort, bypassing allowance restrictions for WL is pretty serious.

Proof of Concept

To mint a token, users would call the MinterContract.mint() function. First, it checks whether number of tokens minter does not exceeds max allowance. Then, it call the mint function in the NextGenCore contract, which calls the _mintProcessing.

    function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
        tokenData[_mintIndex] = _tokenData;
        collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
        tokenIdsToCollectionIds[_mintIndex] = _collectionID;
        _safeMint(_recipient, _mintIndex);
    }

The _safeMint() checks if recipient refers to a smart-contract. If it does, then it makes an external call to its onERC721Received() function, which must return a magic value. However, this allows a minter to reenter MinterContract.mint() function and mint as many tokens as they want since the tokensMintedAllowlistAddress mapping is updated only AFTER the _safeMint() is invoked.

    function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
            if (phase == 1) { //@audit updated after _safeMint() is invoked
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
        }
    }

So this two requirements won't work:

require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens, "AL limit");
...
require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");

Tools Used

Manual Review

Recommended Mitigation Steps

Add the non-reentrant modifier to the MinterContract.mint() function.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

An auction winner can refund his winning bid once, all his other bids twice and get the auctioned token for free if he calls the claimAuction() and cancelAllBids() consecutively at exactly auctionEndTime timestamp. This allows anyone to drain the auctionDemo contract via a flashloan.

Details

Once an admin calls the mintAndAuction() function of the MinterContract.sol, an auction starts. Everyone can participate in it through participateToAuction() function by sending more ETH than the previous bidder sent and until auction end time is reached. When block.timestamp >= auctionEndTime, a winner or an admin can call the claimAuction() function, which iterates through all the bidders, transfers NFT to the winner, sends the winning bid to the NFT owner and refunds all other bids.

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

There are also two functions with similar functionality. The cancelBid() function cancels one of the bidder's bid by setting the bid's status to false and returning ETH that was bid to the bidder. The cancelAllBids() just cancels all bids by iterating through auctionInfoData mapping. These can be called when block.timestamp <= auctionEndTime.

Now, the main problem here is that upon claiming statuses of the bids are not updated. The second problem is that all this three contract's functionalities can be executed at exactly auctionEndTime. So an attacker could create a custom smart contract, which calls participateToAuction(), claimAuction() and cancelAllBids() consecutively in one attack() function. Now, he only waits for timestamp == minter.getAuctionEndTime() to know how much ETH the auctionDemo contract holds. If contract's total balance is twice the winning bid amount (it is needed because bids are refunded starting from the smallest), he takes a flashloan and calls the attack(). The auctionDemo contract would send the NFT to the attacker and the winning bid amount to the owner of NFT in claimAuction(). But since statuses of the bids are not updated there, the next call to cancelAllBids() would "refund" the attacker with the winning bid amount, allowing him to get the NFT for free. Notice that twice the winning bid amount is only possible if there are multiple auctions, i.e. auction1 bids == 40 ETH and auction2 bids == 50 ETH. To win auction1 only 40ETH + 1 wei is needed, so this would be taken from the auction2 bids as refund.

Even worse scenario is possible since all attacker's bids can be refunded twice (apart from the winning bid, which would be refunded only once). Suppose 2 auctions are at place. Bids for the first one equal to 100 ETH. Bids for the second is only 40 ETH in total. The second auction is about to end. The contract holds 140 ETH. The attacker could place two bids - the first one of 49.9 ETH and the second one of 50 ETH. In this case by calling the attack() function he would get the NFT, his 99.9 ETH and 49.9 ETH on top as a second "refund". Notice again that this only works if there are multiple auctions.

Also, an attacker can create a custom fallback function which would query the status of his bid. Upon claiming, an externall unprotected call to it would be made, and if the bid's status == true, the fallback would make a call to cancelBid() function. This way anyone can refund his bids twice if there are exist bids that are higher.

Proof of Concept

https://gist.github.com/seeques/81e1a2a1b230b1f12178200f4bcb539d

    function testDrainContract() public {
        mintTokensAndDeployExploit();

        // 10000 ether in exploit contract
        uint256 initialExploitBalance = address(exploit).balance;
        uint256 minTokenId = core.viewTokensIndexMin(1);
        // user1 sends 40 ether for the second auction
        vm.prank(user1);
        auction.participateToAuction{value: 40 ether}(minTokenId + 1);
        // user2 sends 60 ether to the second auction
        vm.startPrank(user2);
        auction.participateToAuction{value: 60 ether}(minTokenId + 1);
        // user2 sends 40 ether to the target auction
        auction.participateToAuction{value: 40 ether}(minTokenId);
        vm.stopPrank();

        // set the timestamp to the auction endtime
        vm.warp(minter.getAuctionEndTime(minTokenId));

        // attacker places two bids, claims and cancels all bids in one transaction, succesfully draining the contract
        exploit.callClaimToDrain(minTokenId, 49.9 ether, 50 ether);

        // +49.9 ether
        uint256 exploitBalanceAfterClaim = address(exploit).balance;

        bool ethIsStolen = exploitBalanceAfterClaim > initialExploitBalance;

        assertEq(ethIsStolen, true);
        assertEq(IERC721(core).ownerOf(minTokenId), address(exploit));

        console.log("Winner's initial balance:", initialExploitBalance);
        console.log("Winner's balance after claim:", exploitBalanceAfterClaim);
    }

Tools Used

Foundry, manual review

Recommended Mitigation Steps

Update the status of the highest and each refunded bid to true in claimAuction() function. Also, consider separating refunding and claiming functionalities.

Assessed type

Invalid Validation