Royalty Payment Invariant Violation

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/tree/main/smart-contracts/MinterContract.sol#L408 https://github.com/code-423n4/2023-10-nextgen/tree/main/smart-contracts/MinterContract.sol#L418

Vulnerability details

Impact

The vulnerability in the payment mechanism of the smart contract significantly impacts the protocol's functionality. The root cause of the vulnerability is that, despite the README stating an invariant that "Payments can only be made when royalties are set, the artist proposes addresses and percentages, and an admin approves them," poorly managed status control allows the admin to accept addresses and percentages without requiring an artist's proposal. This oversight breaks the stated invariants and has several negative consequences.

// File: README.md
178:- Payments can only be made when royalties are set, the artist proposes addresses and percentages, and an admin approves them. // <= FOUND

Proof of Concept

To demonstrate this vulnerability, an admin can accept addresses and percentages without requiring an artist's proposal, resulting in unauthorized royalty payments to the team wallet.

1. The admin, due to the poorly managed status control, can accept addresses and percentages without waiting for the artist to make a proposal.

   // File: smart-contracts/MinterContract.sol
   408:    function acceptAddressesAndPercentages(uint256 _collectionID, bool _statusPrimary, bool _statusSecondary) public FunctionAdminRequired(this.acceptAddressesAndPercentages.selector) {// <= FOUND: Payments can be made skipping royalties set and the artist proposes.
   409:        collectionArtistPrimaryAddresses[_collectionID].status = _statusPrimary;
   410:        collectionArtistSecondaryAddresses[_collectionID].status = _statusSecondary;
   411:    }

2. This allows the admin to proceed to obtain all royalties to the team wallet via the payArtist function.

   // File: smart-contracts/MinterContract.sol
   415:    function payArtist(uint256 _collectionID, address _team1, address _team2, uint256 _teamperc1, uint256 _teamperc2) public FunctionAdminRequired(this.payArtist.selector) {
   ...
   418:        require(collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage + _teamperc1 + _teamperc2 == 100, "Change percentages");// <= FOUND: artistPercentage is 0 if skipped previous steps. Admin can chose _teamperc1 + _teamperc2 = 100% to obtain all royalties
       ...
   444:    }

3. The protocol's payment functionality is compromised, and the invariant is not upheld.
4. Further, this violation can lead to other scenarios, such as the admin resetting the artist/team split even after approval or an artist front-running the admin's approval with a different proposal against the admin's intention.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, it is recommended to extend the status control mechanism into multiple stages, such as "INIT," "PERCENTAGE_SPLIT_SET," "ARTIST_PROPOSED," and "PERCENTAGE_ACCEPTED," and control the state flow properly. This will ensure that payments are made only when royalties are set, the artist proposes addresses and percentages, and an admin approves them, as specified in the README.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

a2rocket (sponsor) disputed

[alex-ppg]

The Warden specifies that an administrator can "accept" percentages that were never proposed (i.e. full of 0 entries) to take advantage of the payArtist function with their own recipients.

As per the proposePrimaryAddressesAndPercentages and proposeSecondaryAddressesAndPercentages implementations of the MinterContract, both the function administrator and artist members can propose percentages, etc. The Sponsor has evidenced in a separate exhibit that this case is deliberate in case the artist loses access to their account for one reason or another (#522).

As such, I consider this exhibit invalid given that the administrator is considered an entirely trusted party concerning payments made within the payArtist function.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid