NextGen - Delvir0's results

Lines of code

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

Vulnerability details

Impact

Due to improper two aspects, there's a scenario where cancelBid and claimAuction are both callable while it always should be one of the two.

claimAuction() has the following require statement require(block.timestamp >= minter.getAuctionEndTime(_tokenid)) cancelBid() has the following require(block.timestamp <= minter.getAuctionEndTime(_tokenid))

This means that if block.timestamp == minter.getAuctionEndTime(_tokenid) both functions will be activated.

When claimAuction() is called auctionInfoData[_tokenid][i].status is never set to false like it would in cancelBid().

This results in the attack vector where cancelBid() is called after claimAuction() in the same block.timestamp. This could lead to 1. the winner getting the NFT and getting his bid back 2. a bidder receiving his bid twice. An attacker could check the current balance of the contract, deposit the same amount and withdraw that amount * 2 by above mentioned attack.

Proof of Concept

1. minter.getAuctionEndTime(_tokenid) == 100, current block.timestamp == 100
2. claimAuction() is called where the following passes block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true
3. Monitor mempool and submit a cancelBid() transaction and ensure it is mined in the same timestamp
4. cancelBid() passes the following requirements block.timestamp <= minter.getAuctionEndTime(_tokenid) and auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true since the status is never set to false in claimAuction()

Tools Used

manual review

Recommended Mitigation Steps

For this specific issue either fixing cause 1 or 2 will fix this problem. The timestamp checks needs to be checked throughout the whole codebase. I would recommend:

1. Either end actions at the endtime or after the endtime throughout the whole codebase. Example: endOfAuction == endTime here, when block.timestamp hits endTime, the auction would be ended. OR: endOfAcution > endTime here, when block.timestamp hits endTime, the auction is still open and would end when that specific time has been PASSED.

2. set blocktimestamp requirement of AuctionDemo.sol to: require(block.timestamp <= minter.getAuctionEndTime(_tokenid)

3. in claimAcution(), set auctionInfoData[_tokenid][index].status to false BEFORE sending back the ETH or NFT

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Note: 1. this requires the attacker to lose his highest bid and lose out on the NFT 2. unlike the bot report this is not due to the loop DOS.

When the auction is over, claimAuction() loops over the bids and either 1. sends the NFT to the highest bidder and 2. returns the bids to the participants. However, if the highest bidder reverts the transfer on purpose, no one will be able to get their bids value back. This is due to the fact that when calling the function, it will loop over all bids and eventually end up in the transfer call.

Proof of Concept

1. Attacker sends highest bid via a contract
2. Attacker sets _checkOnERC721Received to revert (or does not implement it at all)
3. When the admin calls claimAuction(), it will always revert

Tools Used

Manual review

Recommended Mitigation Steps

There are two ways to handle this.

1. Implement a failsafe for the functions when they revert. This could be done by continuing the function but saving the revert values for a later moment, for example with a try catch. If the try attempt fails, save the values (bid amount and bidder) into a mapping and provide an additional function to retrieve those funds at a later time.
2. Redesign the claimAuction() without the modifier and where the transfer only occurs if msg.sender == auctionInfoData[_tokenid][i].bidder

Assessed type

DoS