A collection may withdraw more funds than allocated due to functions overlaps

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

When initializing splits and percentages in a collection, a misconfiguration during updates may lead to one collection withdrawing more ETH than allocated, causing a loss for other collections.

Note

This is not related to malicious actions or admin errors; it's a misconfiguration issue that can result in the loss of funds for other collections.

Proof of Concept

Issue summary

During the setup of artist team and split percentages, if an artist requested to change the splits after initializing his addresses, it can result in the artist's addresses receiving more funds than intended, leading to a loss of funds for other collections.

Here is a coded PoC to demonstrate the issue:

    function testSplitsMisconfiguration() public {
        bytes32[] memory merkleProof = setUpArrayBytes(1);
        merkleProof[0] = bytes32(0);

        vm.warp(block.timestamp + 1 days);

        hoax(whitelisted1, 5e18);
        minterContract.mint{value: 5e18}({
            _collectionID: 1,
            _numberOfTokens: 5,
            _maxAllowance: 0,
            _tokenData: "",
            _mintTo: whitelisted1,
            merkleProof: merkleProof,
            _delegator: address(0),
            _saltfun_o: 0
        });

        super.collectionTwoSetup();

        hoax(whitelisted2, 10e18);
        minterContract.mint{value: 10e18}({
            _collectionID: 2,
            _numberOfTokens: 5,
            _maxAllowance: 0,
            _tokenData: "",
            _mintTo: whitelisted2,
            merkleProof: merkleProof,
            _delegator: address(0),
            _saltfun_o: 0
        });

        console.log(
            "(Minter contract balance before) =",
            address(minterContract).balance
        );
        console.log(
            "(Collection 1 balance before)    =",
            minterContract.collectionTotalAmount(1)
        );
        console.log(
            "(Collection 2 balance before)    =",
            minterContract.collectionTotalAmount(2)
        );

        vm.prank(genAdminOwner);
        minterContract.setPrimaryAndSecondarySplits({
            _collectionID: 1,
            _artistPrSplit: 90,
            _teamPrSplit: 10,
            _artistSecSplit: 90,
            _teamSecSplit: 10
        });

        vm.prank(artist1);
        minterContract.proposePrimaryAddressesAndPercentages({
            _collectionID: 1,
            _primaryAdd1: artistPyAdd1,
            _primaryAdd2: artistPyAdd2,
            _primaryAdd3: artistPyAdd3,
            _add1Percentage: 30,
            _add2Percentage: 30,
            _add3Percentage: 30
        });

        vm.prank(artist1);
        minterContract.proposeSecondaryAddressesAndPercentages({
            _collectionID: 1,
            _secondaryAdd1: artistSPyAdd1,
            _secondaryAdd2: artistSPyAdd2,
            _secondaryAdd3: artistSPyAdd3,
            _add1Percentage: 30,
            _add2Percentage: 30,
            _add3Percentage: 30
        });

        // Here the artist requested to change the splits

        vm.prank(genAdminOwner);
        minterContract.setPrimaryAndSecondarySplits({
            _collectionID: 1,
            _artistPrSplit: 80,
            _teamPrSplit: 20,
            _artistSecSplit: 80,
            _teamSecSplit: 20
        });

        vm.prank(genAdminOwner);
        minterContract.acceptAddressesAndPercentages({
            _collectionID: 1,
            _statusPrimary: true,
            _statusSecondary: true
        });

        vm.prank(genAdminOwner);
        minterContract.payArtist({
            _collectionID: 1,
            _team1: genAdminOwner,
            _team2: genAdminOwner,
            _teamperc1: 10,
            _teamperc2: 10
        });

        console.log(
            "(Minter contract balance after)  =",
            address(minterContract).balance
        );
        console.log(
            "(Collection 1 balance after)     =",
            minterContract.collectionTotalAmount(1)
        ); // Collection 1 will receive 0.5 ETH more
        console.log(
            "(Collection 2 balance after)     =",
            minterContract.collectionTotalAmount(2)
        ); // Collection 2 will receive 0.5 ETH less
    }

Logs result:

  (Minter contract balance before) : 15000000000000000000
  (Collection 1 balance before)    : 5000000000000000000
  (Collection 2 balance before)    : 10000000000000000000
  (Minter contract balance after)  : 9500000000000000000
  (Collection 1 balance after)     : 0
  (Collection 2 balance after)     : 10000000000000000000

Test Setup:

• Clone the repository: https://github.com/0xbtk/NextGen-Setup.git
• Incorporate the tests in NextGenSecurityReview
• Execute: forge test --mt testSplitsMisconfiguration -vvv

Tools Used

Manual review

Recommended Mitigation Steps

To fix this, you should delete the addresses splits when resetting the team and artist percentages.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

the payArtist function allows to payments to artists between phases. once a payment is made the collectionAmount goes to 0 and then increases while the second minting starts etc.

[c4-judge]

alex-ppg marked the issue as duplicate of #1686

[c4-judge]

alex-ppg marked the issue as satisfactory