NextGen - Vagner's results

Lines of code

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

Vulnerability details

Impact

The auction can be reentered, in some specific situations, which would cause a bidder to get back more ether than it should from the contract.

Proof of Concept

The bid system allows an user that bided already to get his share back by calling cancelBid or cancelAllBids if he bided multiple times. Both of them check if block.timestamp is <= than getAuctionEndTime as can be seen here https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L125 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L135 and if that is the case, it sets the status of the bidder to false https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L127 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L138 so when claimAuction is called, the iteration would pass over the users that already cancel and claim their bids. The problem relies in the fact that claimAuction checks for block.timestamp to be >= than getAuctionEndTime while cancelBid and cancelAllBids check for block.timestamp to be <= than getAuctionEndTime so there is a small timeframe where both statements would be true, when block.timestamp is == to getAuctionEndTime. In that scenario, when claimAuction is called, because block.timestamp is the same trough a whole transaction, a malicious user could have bided with a contract that looks like this

contract MaliciousBidder {
    uint256 tokenID;
    uint256 index;
    address AuctionAddress;

    function participate(uint256 tokenId, address x) public payable {
        Auction(x).participateToAuction{value : msg.value}(tokenId);
    }

    receive () payable external {
        Auction(AuctionAddress).cancelBid(tokenID,index);
    }
}

which would basically call the cancelBid function and all of the checks would pass since block.timestamp would be == to getAuctionEndTime https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L125 msg.sender would be the bidder and the status is still set to true, since it is not changed in the claimAuction function https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L126 In that way the bidder would get twice the amount of ether that he is supposed to, which would make other bidders lose their funds. Since the claimAuction can be called also by the highest bidder, he could call the function exactly at block.timestamp == minter.getAuctionEndTime(_tokenid), in case he bided multiple times, so he can get more ether back thatn it should, so this makes it easily abusable.

Tools Used

Manual review

Recommended Mitigation Steps

There are multiple ways to fix this, you can :

• use a reentrancy protection to the functions
• set the status of the bidders to false before you sent them the ether in the claimAuction function
• check only for block.timestamp to be only < or > without the = sign, so the vulnerability is not achievable

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

The way the auction it is implemented right now in AuctionDemo.sol can be easily DoS'ed blocking the whole claimAuction mechanism.

Proof of Concept

The AuctionDemo.sol is used to auction an NFTcreated in the MinterContract.sol by calling mintAndAuction function. After that, anyone can bid on it by calling participateToAuction https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L57-L61 where you need to pay more than the highest bidder, to get your data saved in the auctionInfoStru. After the auction ends the highest bidder or the admin can call claimAuction to iterate over the whole array of auctionInfoStru saved, and transfer the NFT from the owner to the highest bidder and also transfer the ether sent by any other bidder back to themselves, as can be see here https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L110-L119 The problem relies in the fact that this can be easily abusable, by forcing an OOG error and blocking the whole mechanism. To give you an example, I can participate in the early stages of auction, to pay a very low amount of ether, with a contract created by me which looks like this

contract MaliciousBidder {
    uint256[2000] public arr;

    function participate(uint256 tokenId, address x) public payable {
        Auction(x).participateToAuction{value : msg.value}(tokenId);
    }

    receive () payable external {
        // simulate a very expensive operation
        for (uint256 index = 0; index < 2000; index++) {
            arr[index] = index + 1;
        }
    }
}

and when the claimAuction function is called, because it iterates and tries to transfer the balances to every bidder, this would cost a lot of gas, more than the gas limit of the block which would force an OOG and block the auction forever. To give you some data about it, I tested with and array of 200 items, and that would cost around 4 million gas, only for the receive function, considering others operations/iterations done in the claimAuction this would cause and OOG.

Tools Used

Manual review

Recommended Mitigation Steps

My recommendation would be to use the Pull-over-Push method, let every bidder claim his share, and only transfer the NFT to the highest bidder and the ether to the owner in the claimAuction, in that way you can't force and block the whole functionality of the contract.

Assessed type

DoS