NextGen - 7siech's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L415 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L408 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L369 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L380

Vulnerability details

Impact

NextGenMinterContract.payArtist can unintentionally steal funds from other collections by paying out more than 100% of royalties.

This can happen because the artist's proposed royalties are stored as absolute percentages of total royalties as opposed to relative percentages of the overall split plus the payArtist function also considering the team split as absolute percentages.

Proof of Concept

The following sequence of events can lead to the the payArtist function overpaying:

1. the admin sets an artist vs team split on a collection using setPrimaryAndSecondarySplits, ex 50/50
2. the artist then proposes addresses and percentages using proposePrimaryAddressesAndPercentages, ex 50 for one address
3. the admin now has changed their mind on the split and goes back to change the split using setPrimaryAndSecondarySplits, ex 10/90
4. the admin proceeds to accept the percentages using acceptAddressesAndPercentages locking in the 10/90 split
5. after some time passes and ether has accumulated in the contract, the admin pays the artist using payArtist with the locked in team split, ex 90

In the above scenario, the contract now paid out 140% of royalties.

Detailed explanation and code walkthrough

Going through the payArtist function -

Assuming the example numbers taken above, the artistPercentage is 10 and the _teamperc1 is 90, so the require check passes.

require(collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage + _teamperc1 + _teamperc2 == 100, "Change percentages");

We then proceed to calculate the royalties for the first address which in our example is 50.

artistRoyalties1 = royalties * collectionArtistPrimaryAddresses[colId].add1Percentage / 100;

We then calculate the team roylaties using 90 in our running example.

teamRoyalties1 = royalties * _teamperc1 / 100;

Adding up the percentages, it is easy to see that we have now calculated royalties of 140% (50% artist plus 90% team) resulting in the contract using funds from other collections if available.

Since the results of the low level calls are not used directly, this will always succeed and the artist will first receive any funds before any funds will be paid out to the team. In case there are not enough funds, the calls will simply fail but not revert.

        (bool success1, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd1).call{value: artistRoyalties1}("");
        (bool success2, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd2).call{value: artistRoyalties2}("");
        (bool success3, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd3).call{value: artistRoyalties3}("");
        (bool success4, ) = payable(tm1).call{value: teamRoyalties1}("");
        (bool success5, ) = payable(tm2).call{value: teamRoyalties2}("");

Here is the full example -

    uint artistPrSplit = 50;
    uint teamPrSplit = 100 - artistPrSplit;

    uint artistSecSplit = 50;
    uint teamSecSplit = 50;
    vm.prank(admin);
    nextGenMinterContract.setPrimaryAndSecondarySplits(collectionId, artistPrSplit, teamPrSplit, artistSecSplit, teamSecSplit);

    uint add1Percentage = 50;
    vm.startPrank(artist);
    nextGenMinterContract.proposePrimaryAddressesAndPercentages(collectionId, artist, artist, artist, add1Percentage, 0, 0);
    vm.stopPrank();

    // now go back and change the artist primary split
    artistPrSplit = 10;
    teamPrSplit = 100 - artistPrSplit;

    vm.startPrank(admin);
    nextGenMinterContract.setPrimaryAndSecondarySplits(collectionId, artistPrSplit, teamPrSplit, artistSecSplit, teamSecSplit);

    // accept percentages
    nextGenMinterContract.acceptAddressesAndPercentages(collectionId, true, true);
    vm.stopPrank();

    vm.deal(address(nextGenMinterContract), 1 ether);
    vm.deal(user, 1 ether);

    assertEq(user.balance, 1 ether);
    // assume minter contract holds funds from another collection
    assertEq(address(nextGenMinterContract).balance, 1 ether);

    // user mints
    vm.prank(user);
    bytes32[] memory merkleProof = new bytes32[](1);
    nextGenMinterContract.mint{value: 1 ether}(collectionId, 
                               1, //_numberOfTokens, 
                               1, // _maxAllowance, 
                               "", //_tokenData, 
                               user, //_mintTo,
                               merkleProof, // bytes32[] calldata merkleProof, 
                               address(0), //address _delegator, 
                               0); //uint256 _saltfun_o);
    
    // check balances
    assertEq(user.balance, 0);
    assertEq(team.balance, 0);
    assertEq(artist.balance, 0);
    assertEq(address(nextGenMinterContract).balance, 2 ether);

    // initiate payout
    uint teamperc = 90; 
    vm.startPrank(admin);
    nextGenMinterContract.payArtist(collectionId, team, team, teamperc, 0);
    vm.stopPrank();

    // convince ourselves that we have paid out more than 100%
    // and the minter contract has overpaid the artist 0.4 ether 
    assertEq(user.balance, 0);
    assertEq(team.balance, 0.9 ether);
    assertEq(artist.balance, 0.5 ether);
    assertEq(0.6 ether, address(nextGenMinterContract).balance);

POC https://gist.github.com/alpeware/bd533997ecff0c8fd2b6c48c15f2baec#file-nextgen-t-sol-L239

Logs https://gist.github.com/alpeware/bd533997ecff0c8fd2b6c48c15f2baec#file-log-txt

Tools Used

• Foundry
• Manual review

Recommended Mitigation Steps

The code could be refactored so that proposeSecondaryAddressesAndPercentages would consider the proposed percentages as relative to the artistPercentage thus requiring them to add up to 100%

Instead of

require (_add1Percentage + _add2Percentage + _add3Percentage == collectionRoyaltiesSecondarySplits[_collectionID].artistPercentage, "Check %");

use

require (_add1Percentage + _add2Percentage + _add3Percentage == 100, "Check %");

Then in payArtist, the artist royalties would be calculated first based on the split before the individual amounts are calculated.

uint256 royalties = collectionTotalAmount[_collectionID];
uint256 artistRoylaties = royalties * collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage / 100;
artistRoyalties1 = artistRoyalties * collectionArtistPrimaryAddresses[colId].add1Percentage / 100;
artistRoyalties2 = artistRoyalties * collectionArtistPrimaryAddresses[colId].add2Percentage / 100;
artistRoyalties3 = artistRoyalties * collectionArtistPrimaryAddresses[colId].add3Percentage / 100;

The same logic could also be applied to the team split.

Assessed type

Math