NextGen - Eigenvectors's results

Lines of code

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

Vulnerability details

Summary

A timing vulnerability exists within the auctionDemo contract, where the claimAuction and cancelBid functions can be executed in the same block when block.timestamp == minter.getAuctionEndTime(_tokenid). This concurrency flaw could be exploited by a malicious auction winner (highest bidder) to claim the NFT via claimAuction and simultaneously cancel their bid using cancelBid using a onERC721Received hook. This results in the auction winner receiving the NFT for free and to lost funds for other bidders.

Vulnerability Details

A malicious actor could write a smart contract which bids on an auction. When the auction time ends (block.timestamp == minter.getAuctionEndTime(_tokenid)), the malicious actor calls the bidding smart contract which then calls the claimAuction function of the auctionDemo contract, which transfers the ownership of the NFT to the smart contract using safeTransferFrom. This triggers the onERC721Received function inside the malicious contract which then calls the cancelBid function of the auctionDemo contract, which returns due to missing checks the bid amount to the bidder (smart contract of the malicious actor). As no success checks for transferring the ETH to the owner (payment for the NFT) or the ETH of lower bids back to bidders is present in the claimAuction no reverts happen on insufficient funds which leaves the malicious actor getting the NFT for free and the contract owner or other bidders with lost funds.

A step by step explanation:

Step 1; malicious actor makes bids using a smart contract, with an onERC721Received function that calls cancelBid if it receives an NFT from the auction.

Step 2; if malicious actor has the highest bid at the end of the auction time, malicious actor calls the bidding smart contract, which in turn calls the claimAuction function of the auctionDemo contract.

contract Exploit {
    ...

		// malicious actor calls this function
    function exploit() external {
				// user already checks beforehand if this contract made the highest bid. If not, this call throws an error
        auctionContract.claimAuction(tokenId);
    }
		
		...
}

Step 3; auctionDemo contract transfers the NFT ownership to the bidding smart contract

// auction winner (address with highest bid) or admins can call this function.
function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
	// requires current block.timestamp to be greater or EQUAL to auction end time, auction to be not yet claimed and auction status to be true
  require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
	// mark auction as claimed
  auctionClaim[_tokenid] = true;
	// get highest bid -> in this case the bid from the contract which called this function
  uint256 highestBid = returnHighestBid(_tokenid);
	// get current owner of NFT
  address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
	// get highest bidder -> in this case the contract which called this function
  address highestBidder = returnHighestBidder(_tokenid);
	// iterate over all bids for this NFT
  for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
		// if current bid is the highest bid
    if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
			// then transfer the NFT ownership to the highest bidder -> contract by malicious actor in this case
			// this triggeres the onERC721Received
      IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
			// the rest of the function runs in Step 6
			// NFT owner is payed for NFT
			// note that success of transfer is not required so function does not revert if insufficient funds are available.
      (bool success, ) = payable(owner()).call{value: highestBid}("");
			// emit ClaimAuction event
      emit ClaimAuction(owner(), _tokenid, success, highestBid);
		// else if bid not the highest bid and not canceled
    } else if (auctionInfoData[_tokenid][i].status == true) {
			// return funds to bidder
			// note that success of transfer is not required so function does not revert if insufficient funds are available.
      (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
			// emit Refund event
      emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
    } else {}
  }
}

Step 4; onERC721Received of the bidding smart contract is called which in turn calls cancelBid of the auctionDemo contract (still in the same time block which is equal to auctionTimeEnd).

contract Exploit {
  ...

	// ERC721 standard function which is called after a successful transfer.
  function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) {
    // During the same transaction, cancelBid is called, which refunds the bid amount
    auctionContract.cancelBid(tokenId, bidIndex); // bidIndex to be determined as per auction logic
    return this.onERC721Received.selector;
  }		
	...
}

Step 5; auctionDemo contract cancels bid of smart contract and sends the bid amount back to the bidding smart contract.

// no restrictions on who can call this function
function cancelBid(uint256 _tokenid, uint256 index) public {
	// requires current block.timestamp to be smaller or EQUAL to auction end time
  require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
	// require caller to be the owner of bid at provided index and bid to not yet be canceled
  require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
	// mark bid as canceled
  auctionInfoData[_tokenid][index].status = false;
	// transfers the bid amount from this contract to the caller
	// note that success of transfer is not required so function does not revert if insufficient funds are available.
  (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
	// emit CancelBid event
  emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
}

Step 6; auctionDemo completes the claimAuction function call and pays the NFT seller (highest bid) and transfers the unsuccessful bids back to the bidders. If the auctionDemo contract does not have sufficient funds to return all funds no revert happens because no successful transfer is for function success required. (see code in Step3)

Step 7; malicious actor transfers funds and NFT to himself.

Impact

Auction winner (highest bidder) getting the NFT for free and the bidders from other auctions pay the price for it.

Tools Used

Manual Review

Recommendations

Make cancelBid and claimAuction not be callable at the same time by changing the required block.timestamp to > and < instead of ≥ and ≤.

Assessed type

Timing

Lines of code

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

Vulnerability details

Summary

A timing vulnerability exists within the auctionDemo contract, where the claimAuction and cancelBid functions can be executed in the same block when block.timestamp is equal to minter.getAuctionEndTime(_tokenid). This concurrency flaw could be exploited by a malicious auction winner (highest bidder) with another smaller non-winning bid or any other bidder when winner or admin calls the claimAuction at block.timestamp equal to minter.getAuctionEndTime(_tokenid) by invoking cancelBid in the same transaction using a onReceive function. This results in the bidder receiving twice the amount of their bid back.

Vulnerability Details

A malicious actor could write a smart contract which bids on auctions and has a receive function which calls the cancelBid function on receiving funds. For this to work, the malicious actor needs a bid which is not the winning bid (one can have multiple bids). When the claimAuction function is invoked at block.timestamp equal to minter.getAuctionEndTime(_tokenid) by the auction winner (which can be the malicious actor with another bid) or an admin, the funds of non-winning bids are returned to the bidding contract. The receive function of the malicious contract then calls the cancelBid in turn and receives the funds the second time.

A step by step explanation:

Step 1; malicious actor makes bids of which at least one is not the winning bid using a smart contract with a receive function that calls the cancelBid function on receiving funds from an auction.

Step 2; claimAuction function of the auctionDemo contract is called at block.timestamp == auctionEndTime.

// auction winner (address with highest bid) or admins can call this function.
function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
	// requires current block.timestamp to be greater or EQUAL to auction end time, auction to be not yet claimed and auction status to be true
  require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
	// mark auction as claimed
  auctionClaim[_tokenid] = true;
	// get highest bid
  uint256 highestBid = returnHighestBid(_tokenid);
	// get current owner of NFT
  address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
	// get highest bidder
  address highestBidder = returnHighestBidder(_tokenid);
	// iterate over all bids for this NFT
  for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
		// if current bid is the highest bid
    if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
			// then transfer the NFT ownership to the highest bidder
      IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
			// NFT owner is payed for NFT
			// note that success of transfer is not required so function does not revert if insufficient funds are available.
      (bool success, ) = payable(owner()).call{value: highestBid}("");
			// emit ClaimAuction event
      emit ClaimAuction(owner(), _tokenid, success, highestBid);
		// else if bid not the highest bid and not canceled
    } else if (auctionInfoData[_tokenid][i].status == true) {
			// return funds to bidder
			// note that success of transfer is not required so function does not revert if insufficient funds are available.
			// if current bid is the bid of the bidding smart contract of our malicious actor this triggeres an onReceive function of the smart contract
      (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
			// emit Refund event
      emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
    } else {}
  }
}

Step 4; The onReceive function of the bidding smart contract of the malicious actor is triggered, which in turn calls cancelBid of the auctionDemo contract

contract Exploit {
  ...
	// Fallback function which is called when the contract receives Ether
  fallback() external payable {
	  // Call the cancelBid function on the auction contract
		// MISSING: check that this is only called on the first return of funds and not the second
	  auctionContract.cancelBid(tokenid, bidIndex);
  }
	...
}

Step 5; auctionDemo contract cancels bid of smart contract and sends the bid amount back to the bidding smart contract.

// no restrictions on who can call this function
function cancelBid(uint256 _tokenid, uint256 index) public {
	// requires current block.timestamp to be smaller or EQUAL to auction end time
  require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
	// require caller to be the owner of bid at provided index and bid to not yet be canceled
  require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
	// mark bid as canceled
  auctionInfoData[_tokenid][index].status = false;
	// transfers the bid amount from this contract to the caller
	// note that success of transfer is not required so function does not revert if insufficient funds are available.
  (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
	// emit CancelBid event
  emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
}

Step 6; auctionDemo completes the claimAuction function call and pays the NFT seller (highest bid) and transfers the unsuccessful bids back to the rest bidders. If the auctionDemo contract does not have sufficient funds to return all funds, no revert happens because no successful transfer is for function success required. (see code in Step 2)

Step 7; malicious actor transfers funds of smart contract to himself.

Impact

Bidder left with double the funds put into the auction and lost funds for the auction holder and/or other bidders. As bidder can have multiple bids, he could drain all the funds of the auctionDemo contract except from the funds of this auction, as this would lead to a revert.

Tools Used

Manual Review

Recommendations

Make cancelBid and claimAuction not be callable at the same time by changing the required block.timestamp to > and < instead of ≥ and ≤.

Assessed type

Timing

Lines of code

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

Vulnerability details

Summary

A malicious actor can manipulate an auction by creating a huge bid at the very beginning of an auction making it impossible for others to bid in the auction, as the bid function only allows bidding more than the current highest bid. At the end of the auction (block.timestamp == minter.getAuctionEndTime(_tokenid)) the malicious actor cancels their high bid, makes a small bid of eg 1 wei and claims auction in the same transaction. This results in the NFT being bought for 1 wei even if its worth is much greater.

Vulnerability Details

Right at the start of an auction, the malicious actor makes such a high bid that makes it unattractive or unfeasible for others to bid in the auction. At the end of the auction (block.timestamp == minter.getAuctionEndTime(_tokenid)) the malicious actor cancels their bid, makes a bid of e.g. 1 wei and claims the auction in the same transaction which is possible because of their end time check (respectively being greater or equal to and smaller or equal to).

Step 1; Malicious actor makes a really high bid and as the participateToAuction function only allows bids which are greater than the current bid, prevent other users from participating in the auction:

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

Step 2; At block.timestamp == minter.getAuctionEndTime(_tokenid) the malicious actor calls the cancelBid function removing the unfeasible high bid, makes a new smaller bid of e.g. 1 wei and claims the auction by calling claimAuction:

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

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

Impact

Makes it possible for malicious actors to buy NFTs for 1 wei and prevent other users from participating in the auction.

Tools Used

Manual Review

Recommendations

Making bids even possible if they are smaller than the current highest bid.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Summary

The recipient of the highest bid in an auction should be the owner of the token, as stated out in the main invariants in the C4 description of the protocol. Instead, the funds are sent to the owner of the auctionDemo contract.

Vulnerability Details

One of the main invariants in the C4 description mentions that the recipient of the funds from the highest bidder in an auction should be the owner of the token: The highest bidder will receive the token after an auction finishes, the owner of the token will receive the funds and all other participants will get refunded. This main invariant is broken as the funds are sent to the wrong address. Here, we can see that the funds are instead sent to the owner of the auctionDemo contract by calling the owner() function:

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    ...
    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) {
						...
        } else {}
    }
}

Impact

The wrong address will receive the auction funds and a main invariant is broken.

Tools Used

Manual Review

Recommendations

Change the recipient from owner() to ownerOfToken.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Summary

A new highest bid can be made after the auction has been claimed if the claim happens first, but both actions happen in the same block (block.timestamp == minter.getAuctionEndTime(_tokenid)). This results in the newest bid being locked inside the auctionDemo contract as after auction end time the only way to receive funds back is by calling claimAuction which can not be called anymore, as it was already claimed.

Vulnerability Details

The auctionDemo contract is susceptible to a timing loophole, where bids can be placed at the same block timestamp when the auction ends and is claimed. The claimAuction function does not lock out subsequent bids within the same block, leading to a scenario where a bid submitted after claimAuction is accepted and locked in the contract with no means of retrieval post-auction claim.

Timeline for this would be (all at the same block.timestamp):

1. auction is claimed by auction winner or admin calling claimAuction.

// function can be called by auction winner or admin
function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
	// require block.timestamp >= auction end time and auction not yet claimed -> in this case block.timestamp == auction end time
  require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
	// mark auction as claimed
  auctionClaim[_tokenid] = true;
	// return all non-winning bids, transfers ownership of NFT to auction winner and pays previous NFT owner the winning bid
	...
}

2. new bidder bids

// callable by everyone
function participateToAuction(uint256 _tokenid) public payable {
	// require new bid to be bigger then previous bid and block.timestamp to be <= auction end time and auction status to be true in minter contract (not set to false by claiming auction) -> on this case block.timestamp == auction end time
  require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
	// create new bid struct
  auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
	// store new bid
  auctionInfoData[_tokenid].push(newBid);
}

Now the funds of this bid are locked forever as claimAuction can not be called anymore without error as auctionClaim[_tokenid] == true and the two other functions to cancel bids (cancelBid and cancelAllBids) can only be called when block.timestamp is smaller equal auction end time.

Impact

Loss of funds for newest highest bidder when bidding at exactly the end of the auction, but auction was already claimed. Also, a main invariant given by the protocol in the C4 description is broken, as not all participants will receive the refund. The main invariant: The highest bidder will receive the token after an auction finishes, the owner of the token will receive the funds and all other participants will get refunded.

Tools Used

Manual Review

Recommendations

Making it impossible to make bids when auction was already claimed.

Assessed type

Invalid Validation