The `Transfer` event is emitted successfully in `MinterContract#mintAndAuction()` even when the transaction has failed, leading to inaccurate accounting in off-chain systems.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L282-L294

Vulnerability details

Impact

The airDropTokens() function in the NextGenCore contract is responsible for minting and transferring an NFT to a user, exclusively called by the MinterContract. However, the execution of airDropTokens() within MinterContract#mintAndAuction() prior to critical calculations results in the emission of a successful Transfer event, even if the transaction is eventually reverted due to errors in calculations. This inconsistency can lead to accounting issues in off-chain systems that rely on events for synchronization with the smart contract.

Proof of Concept

The Transfer event is emitted on line 282 even when the require statement on line 294 fails.

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L282C8-L294C44

282:@>      gencore.airDropTokens(mintIndex, _recipient, _tokenData, _saltfun_o, _collectionID);
283:        uint timeOfLastMint;
284:        // check 1 per period
285:        if (lastMintDate[_collectionID] == 0) {
286:        // for public sale set the allowlist the same time as publicsale
287:            timeOfLastMint = collectionPhases[_collectionID].allowlistStartTime - collectionPhases[_collectionID].timePeriod;
288:        } else {
289:            timeOfLastMint =  lastMintDate[_collectionID];
290:        }
291:        // uint calculates if period has passed in order to allow minting
292:        uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[_collectionID].timePeriod;
293:        // users are able to mint after a day passes
294:@>      require(tDiff>=1, "1 mint/period");

Test

import "forge-std/Test.sol";
import "../../smart-contracts/NextGenCore.sol";
import "../../smart-contracts/NextGenAdmins.sol";
import "../../smart-contracts/RandomizerNXT.sol";
import "../../smart-contracts/XRandoms.sol";
import "../../smart-contracts/MinterContract.sol";
import "../../smart-contracts/NFTdelegation.sol";
import "../../smart-contracts/AuctionDemo.sol";
import "../../smart-contracts/ERC721.sol";

contract MintAuction is Test {
    NextGenCore public nextGenCore;
    NextGenAdmins public nextGenAdmin;
    NextGenRandomizerNXT public nextGenRandomizerNXT;
    randomPool public xRandoms;
    NextGenMinterContract public nextGenMinter;
    DelegationManagementContract public delegationManager;
    auctionDemo public auctionContract;

    address public owner;
    address public admin;
    address public artist;

    string _collectionName;
    string _collectionArtist;
    string _collectionDescription;
    string _collectionWebsite;
    string _collectionLicense;
    string _collectionBaseURI;
    string _collectionLibrary;
    string[] _collectionScript;

    function setUp() public virtual {
        owner = vm.addr(0xA11CE);
        admin = vm.addr(0xB44DE);
        artist = vm.addr(3);

        vm.deal(admin, 10 ether);

        vm.prank(owner);
        nextGenAdmin = new NextGenAdmins();
        nextGenCore = new NextGenCore(
            "ART Token",
            "ART",
            address(nextGenAdmin)
        );
        xRandoms = new randomPool();
        nextGenRandomizerNXT = new NextGenRandomizerNXT(
            address(xRandoms),
            address(nextGenAdmin),
            address(nextGenCore)
        );

        delegationManager = new DelegationManagementContract();
        nextGenMinter = new NextGenMinterContract(
            address(nextGenCore),
            address(delegationManager),
            address(nextGenAdmin)
        );

        auctionContract = new auctionDemo(address(nextGenMinter), address(nextGenCore), address(nextGenAdmin));

        vm.prank(owner);
        nextGenCore.addMinterContract(address(nextGenMinter));
        vm.stopPrank();

        _collectionName = "Test Collection 1";
        _collectionArtist = "Artist 1";
        _collectionDescription = "For testing";
        _collectionWebsite = "www.test.com";
        _collectionLicense = "CCO";
        _collectionBaseURI = "https://ipfs.io/ipfs/hash/";
        _collectionLibrary = "";
        _collectionScript = ["desc"];
    }

    function createCollection(address user) public {
        vm.prank(user);
        nextGenCore.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );
        vm.stopPrank();
    }

    function registerFunctionAdmin(address user, bytes4 selector) public {
        vm.prank(owner);
        nextGenAdmin.registerFunctionAdmin(address(user), selector, true);
        vm.stopPrank();
    }

    function testAuction() public {

        // Registering function admin
        registerFunctionAdmin(
            address(admin),
            nextGenCore.setCollectionData.selector
        );

        registerFunctionAdmin(
            address(admin),
            nextGenCore.createCollection.selector
        );

        registerFunctionAdmin(
            address(admin),
            nextGenCore.addRandomizer.selector
        );

        registerFunctionAdmin(
            address(admin),
            nextGenMinter.setCollectionCosts.selector
        );

        registerFunctionAdmin(
            address(admin),
            nextGenMinter.setCollectionPhases.selector
        );

        registerFunctionAdmin(
            address(admin),
            nextGenMinter.mintAndAuction.selector
        );

        uint256 collectionId = nextGenCore.newCollectionIndex();

        // Step 1: Create collection
        createCollection(address(admin));

        // Step 2: Set collection data
        vm.prank(admin);
        nextGenCore.setCollectionData(
            collectionId,
            address(artist),
            2,
            10000000000,
            0
        );

        // Step 3: Add randomizer
        vm.prank(admin);
        nextGenCore.addRandomizer(collectionId, address(nextGenRandomizerNXT));

        // Step 4: Set collection costs
        vm.prank(admin);
        nextGenMinter.setCollectionCosts(
            collectionId, // _collectionID
            1 ether, // _collectionMintCost 1 eth
            1 ether, // _collectionEndMintCost 0.1 eth
            0, // _rate
            1, // _timePeriod
            1, // _salesOptions
            address(delegationManager) // delAddress
        );

        // Step 5: Set collection phases
        vm.prank(admin);
        nextGenMinter.setCollectionPhases(
            collectionId, // _collectionID
            block.timestamp, // _allowlistStartTime
            block.timestamp, // _allowlistEndTime
            block.timestamp, // _publicStartTime
            block.timestamp + 1 hours, // _publicEndTime
            0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870 // _merkleRoot
        );

        address _recipient = address(artist);
        string memory _tokenData = '{"tdh": "100"}';
        uint256 _saltfun_o = 2;
        uint _auctionEndTime = block.timestamp + 2 hours;

        // Step 6: Mint
        vm.prank(admin);
        nextGenMinter.mintAndAuction(
            _recipient,
            _tokenData,
            _saltfun_o,
            collectionId,
            _auctionEndTime
        );

        vm.prank(admin);
        vm.expectRevert("1 mint/period");
        nextGenMinter.mintAndAuction(
            _recipient,
            _tokenData,
            _saltfun_o,
            collectionId,
            _auctionEndTime
        );
    }
}

Test Result

    ├─ [207983] NextGenMinterContract::mintAndAuction(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, {"tdh": "100"}, 2, 1, 7201) 
    │   ├─ [856] NextGenAdmins::retrieveFunctionAdmin(0xb742c2a92B070997Def5fB9e125039a4498834D9, 0x46372ba600000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ [506] NextGenCore::retrievewereDataAdded(1) [staticcall]
    │   │   └─ ← true
    │   ├─ [534] NextGenCore::viewCirSupply(1) [staticcall]
    │   │   └─ ← 1
    │   ├─ [555] NextGenCore::viewTokensIndexMin(1) [staticcall]
    │   │   └─ ← 10000000000 [1e10]
    │   ├─ [534] NextGenCore::viewTokensIndexMax(1) [staticcall]
    │   │   └─ ← 19999999999 [1.999e10]
    │   ├─ [534] NextGenCore::viewCirSupply(1) [staticcall]
    │   │   └─ ← 1
    │   ├─ [555] NextGenCore::viewTokensIndexMin(1) [staticcall]
    │   │   └─ ← 10000000000 [1e10]
    │   ├─ [197531] NextGenCore::airDropTokens(10000000001 [1e10], 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, {"tdh": "100"}, 2, 1) 
    │   │   ├─ [35223] NextGenRandomizerNXT::calculateTokenHash(1, 10000000001 [1e10], 2) 
    │   │   │   ├─ [558] randomPool::randomNumber() [staticcall]
    │   │   │   │   └─ ← 897
    │   │   │   ├─ [8912] randomPool::randomWord() [staticcall]
    │   │   │   │   └─ ← Tangerine
    │   │   │   ├─ [22851] NextGenCore::setTokenHash(1, 10000000001 [1e10], 0x5eac5f74ec9ac04ccd8f86fa8c833bf0c91caeb13246e294d48dd1b2443d919d) 
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← ()
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, tokenId: 10000000001 [1e10])
    │   │   └─ ← ()
    │   └─ ← "1 mint/period"
    └─ ← ()

See, this function emit emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, tokenId: 10000000001 [1e10]) event, even if the transaction has failed

Tools Used

Manual review and Foundry

Recommended Mitigation Steps

Invoke airDropTokens after the crucial operations to ensure that the Transfer event is triggered only when the calculations are successful.

Assessed type

ERC721

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

as reported on the bot racer report we will add checks to make sure transaction passed

[alex-ppg]

The Warden's submission is incorrect as an event will not be emitted when a transaction reverts. Events are state-changing and thus are reverted when a transaction fails. The PoC provided utilizes a special expectRevert system which will force the transaction to "not fail".

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid