Centralization concerns for artist as admin have the power to propose and apporve artist address.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L394
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L408

Vulnerability details

Impact

Implementing access control to limit certain functions to trusted admin/protocol addresses makes sense. However, it becomes problematic when functions meant for individual users(artists) can also be controlled by the protocol admin.

In the current setup, artists propose their payment addresses (primary and secondary) and percentages, and then the admin approves the address and percentage that has been set by the artist. This approval is required for the artist to get royalties sent.

However, proposePrimaryAddressesAndPercentages() and proposeSecondaryAddressesAndPercentages(), have a modifier ArtistOrAdminRequire which gives power/liberty to the admin to also propose addresses for the artist. I believe this to be problematic for artists, while it is acceptable for the admin to influence certain functions of the protocol, it is not acceptable for the admin to have the sole power of proposing addresses for artists(users) and approving them.

This makes it possible for the admin to decide to disapprove a proposed address by a user(artist), propose addresses of choice, and approve it immediately, which creates trust issues between the artist and protocol admin, thereby eliminating a core benefit of smart contract which is to eliminate trust concerns for users(artist). Admin might be a trusted entity to the protocol but not to the artist(user).

Also, the design pattern is flawed, first artist have to propose addresses and percentage before admins approves it. This process can be simplified by giving artist the liberty to set primary/secondary addresses and percentages, all that should be added are sanity checks for the percentages(min and max value for each percentage should be maintained while being set by artist). In a separate submission, I highlighted the risk of pushing payment to artist, so artist should be allowed to pull out funds when they have it.

Proof of Concept

ArtistOrAdminRequired modifier allows the admin to influence artist's payment address.

    // certain functions can only be called by an admin or the artist
    modifier ArtistOrAdminRequired(uint256 _collectionID, bytes4 _selector) {
      require(msg.sender == gencore.retrieveArtistAddress(_collectionID) || adminsContract.retrieveFunctionAdmin(msg.sender, _selector) == true || adminsContract.retrieveGlobalAdmin(msg.sender) == true, "Not allowed");
      _;
    }

Tools Used

Manual review

Recommended Mitigation Steps

Update the modifier to allow only the collection artist to set primary and secondary addresses.
Add sanity checks to the percentage setting(max and min acceptable value) and remove the need for admin to call acceptAddressesAndPercentages()

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as duplicate of #584

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

this is the intended design as an artist may lose access to his wallet.

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-judge]

alex-ppg marked the issue as duplicate of #1996

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid