NextGen - Myd's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L383 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L380 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L408 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L383 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L409 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L83-L84

Vulnerability details

Impact

Payments to artist addresses could be redirected by updating the splits/addresses after already accepting them. The proposePrimaryAddressesAndPercentages function can update the primary artist payment addresses even after they have been accepted.

This allows redirecting owed royalty payments away from the original intended recipient.

Bug Description

Allowing the primary artist addresses to be updated after acceptance is a critical flaw that compromises the integrity of the entire royalty payment system.

Here is my assessment of the issue:

• proposePrimaryAddressesAndPercentages directly sets the payment addresses and percentages, but does not check the status before making changes.

• So it can update critical payment data even after the artist has accepted the addresses by setting status = true.

• This allows redirecting payments away from intended recipients after acceptance.

• The ArtistOrAdminRequired modifier also gives overly broad access rather than restricting this sensitive function.

• In the attack scenarios, I demonstrate how this could be exploited by a privileged attacker or even tricking the artist by quickly making changes after acceptance.

My recommendation would be:

• Check status in proposePrimaryAddressesAndPercentages before making changes

• Restrict access to payment related functions only to the artist

• Emit events on address changes so artists can monitor for unauthorized modifications

Proof of Concept

proposePrimaryAddressesAndPercentages allows setting the payment addresses and percentages for a collection via: MinterContract.sol#Line 383

collectionArtistPrimaryAddresses[_collectionID].primaryAdd1 = _primaryAdd1;

• It stores them in the collectionArtistPrimaryAddresses struct.

• The status boolean indicates whether these addresses have been accepted.

• However, there is no check of status before allowing the addresses to be updated.

So, the vulnerability is that proposePrimaryAddressesAndPercentages can still change the payment addresses even after the artist has accepted them by setting status = true. This could lead to redirecting royalty payments away from the intended recipient addresses.

The vulnerability is in these functions: MinterContract.sol#proposePrimaryAddressesAndPercentages MinterContract.sol#acceptAddressesAndPercentages

function proposePrimaryAddressesAndPercentages(

  address _primaryAdd1,
  
  // ...
  
) public 

function acceptAddressesAndPercentages(

  // ...
  
  bool _statusPrimary,
  
  // ...
  
) public

proposePrimaryAddressesAndPercentages sets the payment addresses: MinterContract.sol#Line 383

collectionArtistPrimaryAddresses[_collectionID].primaryAdd1 = _primaryAdd1;

acceptAddressesAndPercentages sets status = true: MinterContract.sol#Line 409

collectionArtistPrimaryAddresses[_collectionID].status = _statusPrimary;

However, proposePrimaryAddressesAndPercentages can still be called after acceptance to update addresses.

This allows updating the mapping storing addresses: MinterContract.sol#Line 84

mapping (uint256 => collectionPrimaryAddresses) private collectionArtistPrimaryAddresses;

So original address:

0x1y3b...f16v

Could be changed to:

0x56ul...9cb4

Redirecting royalty payments.

And it's all because of proposePrimaryAddressesAndPercentages lacks a status check: MinterContract.sol#proposePrimaryAddressesAndPercentages

// Should check status 
function proposePrimaryAddressesAndPercentages() public {

} 

This oversight enables changing addresses post-acceptance.

Here's an in-depth analysis of the potential issues and attack vectors related to this payment address vulnerability.

There are two core weaknesses that enable this vulnerability:

1. No status check in proposePrimaryAddressesAndPercentages

The proposePrimaryAddressesAndPercentages function directly sets the payment addresses and percentages:

function proposePrimaryAddressesAndPercentages() public {

  collectionArtistPrimaryAddresses[_collectionID].primaryAdd1 = _primaryAdd1;
  
  // ...
  
}

However, it does not check that status is false before making changes. This oversight allows addresses to be updated even after acceptance.

2. Overly broad access control

The ArtistOrAdminRequired modifier allows the artist, owner or any admin to call the function:

modifier ArtistOrAdminRequired() {

  // artist, owner, admin can call
  
}

So there is no restriction on who can modify critical payment data.

The weaknesses enable two main attack scenarios:

Malicious Admin/Owner

A privileged role like the owner or admin could:

1. Wait for artist acceptance of addresses

2. Call proposePrimaryAddressesAndPercentages to redirect payments

3. Royalties now go to the wrong address

Tricking the Artist

An attacker could also:

1. Get the artist to accept addresses very early in the process

2. Quickly call proposePrimaryAddressesAndPercentages before launch

3. Artist is unaware payments are redirected

Tools Used

Manual

Recommended Mitigation Steps

Should lock after accept. Add require check on status.

require(status == false, "Already accepted");

This locks in the addresses after acceptance.

OR

add:

require(collectionArtistPrimaryAddresses[_collectionID].status == false, "Already accepted");

To prevent updating the addresses after acceptance.

Assessed type

Access Control