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

5 stars 3 forks source link

The absence of sanity checks in the `MinterContract#mintAndAuction()` function can lead to avoidable error scenarios. #1980

Closed c4-submissions closed 12 months ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The setCollectionCosts() function within the MinterContract is utilized to establish the collection costs and sales model for an upcoming collection sale. As outlined in the documentation, the expectation is that one token can be minted and auctioned during each time period. Therefore, it is crucial to invoke the setCollectionCosts() function and specify a non-zero time period before executing mintAndAuction().

However, a vulnerability exists in the mintAndAuction() contract as it fails to ensure that the time period is greater than zero. This flaw can result in a division by zero error when the time period is zero. Additionally, an arithmetic underflow error may occur if the allowlistStartTime is not set in the setCollectionPhases() function.

Proof of Concept

  1. Line 287 can cause arithmetic underflow error if allowlistStartTime is not set
  2. Line 292 can cause a division by zero error when the time period is zero

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

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;
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
            0, // _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
        );
    }
}

Test Result

    ├─ [268855] 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]
    │   │   └─ ← 0
    │   ├─ [555] NextGenCore::viewTokensIndexMin(1) [staticcall]
    │   │   └─ ← 10000000000 [1e10]
    │   ├─ [534] NextGenCore::viewTokensIndexMax(1) [staticcall]
    │   │   └─ ← 19999999999 [1.999e10]
    │   ├─ [534] NextGenCore::viewCirSupply(1) [staticcall]
    │   │   └─ ← 0
    │   ├─ [555] NextGenCore::viewTokensIndexMin(1) [staticcall]
    │   │   └─ ← 10000000000 [1e10]
    │   ├─ [256331] NextGenCore::airDropTokens(10000000000 [1e10], 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, {"tdh": "100"}, 2, 1) 
    │   │   ├─ [43723] NextGenRandomizerNXT::calculateTokenHash(1, 10000000000 [1e10], 2) 
    │   │   │   ├─ [558] randomPool::randomNumber() [staticcall]
    │   │   │   │   └─ ← 897
    │   │   │   ├─ [8912] randomPool::randomWord() [staticcall]
    │   │   │   │   └─ ← Tangerine
    │   │   │   ├─ [22851] NextGenCore::setTokenHash(1, 10000000000 [1e10], 0x64555ac2feade9bad70b104b2d9a08caa47916a21c544381bade3a49b5496a58) 
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← ()
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, tokenId: 10000000000 [1e10])
    │   │   └─ ← ()
    │   └─ ← "Division or modulo by 0"
    └─ ← "Division or modulo by 0"

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.95ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/foundry/MintAuction2.t.sol:MintAuction
[FAIL. Reason: Division or modulo by 0] testAuction() (gas: 1104932)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual Review and Foundry

Recommended Mitigation Steps

Implement necessary sanity checks to avoid error and unnecessary situations.

Assessed type

Other

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #478

c4-judge commented 12 months ago

alex-ppg marked the issue as not a duplicate

c4-judge commented 12 months ago

alex-ppg marked the issue as primary issue

alex-ppg commented 12 months ago

The Warden and all duplicate exhibits specify that the absence of a time period would cause certain functionality of the protocol to be inoperable that directly relies on its presence.

The time period can be arbitrarily re-configured and may not be necessary depending on the sale type of the collection, meaning that this exhibit is invalid.

This particular Warden also specifies that a zero value allowListStartTime would cause failures and the same as above applies.

c4-judge commented 12 months ago

alex-ppg marked the issue as unsatisfactory: Invalid

alex-ppg commented 11 months ago

Based on the judgment of #2033, I consider submissions #1980 and #1831 to be of QA (NC) rather than invalid and am marking them with the correct overinflated severity tag given that they would be graded C.

To note, #2033 was marked as QA before going through the QA reports and would have been marked with overinflated severity as well given that all collection misconfiguration submissions have been marked as NC due to the possibility of reconfiguration.

c4-judge commented 11 months ago

alex-ppg marked the issue as unsatisfactory: Overinflated severity