MinterContract::payArtist can result in double the intended payout

[c4-submissions]

Lines of code

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

Vulnerability details

Description

The royalty allocation within the protocol works like this:

First an admin sets the split between team and artist. Here it is validated that the artist and team allocations together fill up 100%:

MinterContract::setPrimaryAndSecondarySplits:

File: smart-contracts/MinterContract.sol

369:    function setPrimaryAndSecondarySplits(uint256 _collectionID, uint256 _artistPrSplit, uint256 _teamPrSplit, uint256 _artistSecSplit, uint256 _teamSecSplit) public FunctionAdminRequired(this.setPrimaryAndSecondarySplits.selector) {
370:        require(_artistPrSplit + _teamPrSplit == 100, "splits need to be 100%");
371:        require(_artistSecSplit + _teamSecSplit == 100, "splits need to be 100%");
372:        collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage = _artistPrSplit;
373:        collectionRoyaltiesPrimarySplits[_collectionID].teamPercentage = _teamPrSplit;
374:        collectionRoyaltiesSecondarySplits[_collectionID].artistPercentage = _artistSecSplit;
375:        collectionRoyaltiesSecondarySplits[_collectionID].teamPercentage = _teamSecSplit;
376:    }

The artist can then propose a split between artist addresses, where it is validated that the different artist addresses together add up to the total artist allocation:

MinterContract::proposePrimaryAddressesAndPercentages:

File: smart-contracts/MinterContract.sol

380:    function proposePrimaryAddressesAndPercentages(uint256 _collectionID, address _primaryAdd1, address _primaryAdd2, address _primaryAdd3, uint256 _add1Percentage, uint256 _add2Percentage, uint256 _add3Percentage) public ArtistOrAdminRequired(_collectionID, this.proposePrimaryAddressesAndPercentages.selector) {
381:        require (collectionArtistPrimaryAddresses[_collectionID].status == false, "Already approved");
382:        require (_add1Percentage + _add2Percentage + _add3Percentage == collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage, "Check %");

Then, also when paid out, there is a validation that the split between team and artist allocation adds up:

MinterContract::payArtist:

File: smart-contracts/MinterContract.sol

415:    function payArtist(uint256 _collectionID, address _team1, address _team2, uint256 _teamperc1, uint256 _teamperc2) public FunctionAdminRequired(this.payArtist.selector) {
416:        require(collectionArtistPrimaryAddresses[_collectionID].status == true, "Accept Royalties");
417:        require(collectionTotalAmount[_collectionID] > 0, "Collection Balance must be grater than 0");
418:        require(collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage + _teamperc1 + _teamperc2 == 100, "Change percentages");

The issue is that, here it compares to the artist allocation from artist/team allocations. When the actual amount paid out is calculated, it uses the proposed (and accepted) artist address percentages:

MinterContract::payArtist:

File: smart-contracts/MinterContract.sol

429:        artistRoyalties1 = royalties * collectionArtistPrimaryAddresses[colId].add1Percentage / 100;
430:        artistRoyalties2 = royalties * collectionArtistPrimaryAddresses[colId].add2Percentage / 100;
431:        artistRoyalties3 = royalties * collectionArtistPrimaryAddresses[colId].add3Percentage / 100;

Hence, there is a possibility that up to double the intended amount can be paid out, imagine this scenario:

1. An admin sets artist/team allocation to 100% artist, 0% team.
2. Artist proposes a split between addresses (for the total 100%), which is accepted
3. Admin then changes the allocation to 0% artist, 100% team.

They then call payArtist with the 100% team allocation. This will pass the check as artistPercentage will be 0. However, the artist payouts will use the already proposed artist address allocations. Hence double the amount will be paid out.

Impact

An admin can maliciously or by mistake manipulate the percentage allocation for a collections royalty to pay out up to double the amount of royalty. This could make it impossible to payout royalty later as this steals royalty from other collections.

Proof of Concept

PoC using foundry. Save file in hardhat/test/MinterContractTest.t.sol, also needs forge-std in hardhat/lib:

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.19;

import {Test} from "../lib/forge-std/src/Test.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {INextGenCore} from "../smart-contracts/INextGenCore.sol";

contract MinterContractTest is Test {
  
  uint256 constant colId = 1;
  
  address team = makeAddr('team');
  address artist = makeAddr('artist');

  address core  = makeAddr('core');
  address delegate = makeAddr('delegate');

  NextGenAdmins admin = new NextGenAdmins();
  NextGenMinterContract minter;

  function setUp() public {
    minter = new NextGenMinterContract(core, delegate, address(admin));

    vm.mockCall(core, abi.encodeWithSelector(INextGenCore.retrievewereDataAdded.selector), abi.encode(true));
    vm.mockCall(core, abi.encodeWithSelector(INextGenCore.viewMaxAllowance.selector), abi.encode(1));
    vm.mockCall(core, abi.encodeWithSelector(INextGenCore.retrieveTokensMintedPublicPerAddress.selector), abi.encode(0));
    vm.mockCall(core, abi.encodeWithSelector(INextGenCore.viewTokensIndexMin.selector), abi.encode(1));
    vm.mockCall(core, abi.encodeWithSelector(INextGenCore.viewCirSupply.selector), abi.encode(0));
    vm.mockCall(core, abi.encodeWithSelector(INextGenCore.viewTokensIndexMax.selector), abi.encode(1_000));
    vm.mockCall(core, abi.encodeWithSelector(INextGenCore.retrieveArtistAddress.selector), abi.encode(artist));
    vm.mockCall(core, abi.encodeWithSelector(INextGenCore.mint.selector), new bytes(0));

    minter.setCollectionCosts(colId, 1 ether, 1 ether, 0, 100, 0, address(0));
    minter.setCollectionPhases(colId, 0, 0, 1, 101, bytes32(0));

    vm.warp(1); // jump one sec to enter public phase
  }

  function testFaultySplitResultsInDoubleRoyalty() public {
    // needs to hold one extra eth for payout
    vm.deal(address(minter),1 ether);

    // mint to increase collectionTotalAmount
    minter.mint{value: 1 ether}(colId, 1, 1, '', address(this), new bytes32[](0), address(0), 0);
    assertEq(2 ether, address(minter).balance);

    // begin with setting artist split to 100%, team 0%
    minter.setPrimaryAndSecondarySplits(colId,
      100, 0, // primary
      100, 0  // secondary (not used)
    );

    // set the actual artist split
    minter.proposePrimaryAddressesAndPercentages(colId,
      artist, address(0), address(0),
      100, 0, 0
    );
    minter.acceptAddressesAndPercentages(colId, true, true);

    // set 100% to team, 0% to artist without changing artist address allocation
    minter.setPrimaryAndSecondarySplits(colId,
      0, 100, // primary
      0, 100  // secondary (not used)
    );

    // when artist is paid, 2x the amount is paid out
    minter.payArtist(colId, team, address(0), 100, 0);

    // team gets 1 eth
    assertEq(1 ether, team.balance);
    // artist gets 1 eth
    assertEq(1 ether, artist.balance);
  }
}

Tools Used

Manual audit

Recommended Mitigation Steps

Consider verifying that the artist allocations add up to the artist percentage when calling payArtist.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #303

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as primary issue

[alex-ppg]

The Warden specifies that an overpayment of artist funds can be performed, resulting in fund loss for the system if the artist's percentage is ever reduced after having been configured.

The submission is correct and the core flaw lies in that the MinterContract::payArtist function will utilize the proposed percentages instead of the accepted ones when performing the transfers, incorrectly assuming that they equal the collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage originally enforced when they were set.

The Warden's submission was selected as the best because it details the attack vector precisely, it includes an informative PoC, and provides an easy-to-follow textual description of the issue.

I consider this to be a valid medium-severity issue as the fund loss can tap into the funds of other users and potentially the teams themselves given that the royalties for artists are distributed before the royalties for teams.

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[mcgrathcoutinho]

Hi @alex-ppg, here is why I believe this issue in not valid:

1. The issue assumes there will be a deviation from the original process of how royalties are set. That is, setPrimaryAndSecondarySplits() => proposePrimaryAddressesAndPercentages() => acceptAddressesAndPercentages.
2. The warden mentions that the admin would be malicious or could input by mistake. Such instances are considered reckless admin mistakes as per the C4 SC verdict in the docs
3. There is no incorrect assumption being made by the NextGen team here since the process mentioned in point 1 is to be followed every time royalties are changed. See Discord comment by sponsor

Thank you for taking the time to read this.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for contributing to this particular submission's PJQA process! Let me get back to each of your points:

Point 1

The issue assumes that the artist is willing to break the flow which is a valid assessment.

Point 2

The Warden's submission does not rely on an egregious error or reckless mistake. It also does not rely on their input being incorrect maliciously as any update to the function will cause the bug to manifest at varying degrees. Invoking setPrimaryAndSecondarySplits, as its name implies, should overwrite the split percentages but it does not. This is a reasonable expectation of the function.

Point 3

The submission entails incorrect accounting in the MinterContract rather than an administrative error. Additionally, its impact is significant as it can touch into the funds of other collections as all funds are stored under a single contract.

Conclusion

Combining the facts that this is a critical vulnerability with an acceptable chance of happening, I consider a medium-risk rating appropriate for it.

Keep in mind that the payArtist function, while an administrative function, can be assigned to function-level administrators. Contrary to functions like emergencyWithdraw which clearly denote their purpose, the NextGen team will never reasonably expect that payArtist can result in the contract's funds being siphoned.

As such, the accounting flaw must be corrected and constitutes a medium-risk vulnerability that should be rectified.