Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
c4-submissions opened this issue Nov 13, 2023 · 7 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working primary issue Highest quality submission among a set of duplicates unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-submissions
Copy link
Contributor

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-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 13, 2023
c4-submissions added a commit that referenced this issue Nov 13, 2023
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #478

@c4-judge
Copy link

c4-judge commented Dec 1, 2023

alex-ppg marked the issue as not a duplicate

@c4-judge
Copy link

c4-judge commented Dec 1, 2023

alex-ppg marked the issue as primary issue

@alex-ppg
Copy link

alex-ppg commented Dec 4, 2023

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 c4-judge closed this as completed Dec 4, 2023
@c4-judge
Copy link

c4-judge commented Dec 4, 2023

alex-ppg marked the issue as unsatisfactory:
Invalid

@alex-ppg
Copy link

alex-ppg commented Dec 8, 2023

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
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as unsatisfactory:
Overinflated severity

This was referenced Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working primary issue Highest quality submission among a set of duplicates unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants