search

code-423n4 / 2023-10-nextgen-findings

5 stars 3 forks source link

artist royalty calculated susceptible to wrong calculation #1970

Closed c4-submissions closed 7 months ago

c4-submissions commented 7 months ago

Lines of code

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

Vulnerability details

Impact

This can lead to for the artist during royalty payment.

Proof of Concept

In the payArtist(...) function in the MinterContract.sol contract, the artistRoyalties1, artistRoyalties2 and artistRoyalties3 are susceptible to errors due to missing validation checks. For instance,

But as shown below, these checke is not done and for that reason, the calculated values for artistRoyalties1, artistRoyalties2 and artistRoyalties3 could be susceptible to errors

function payArtist(uint256 _collectionID, address _team1, address _team2, uint256 _teamperc1, uint256 _teamperc2) public FunctionAdminRequired(this.payArtist.selector) {
        require(collectionArtistPrimaryAddresses[_collectionID].status == true, "Accept Royalties");
        require(collectionTotalAmount[_collectionID] > 0, "Collection Balance must be grater than 0");
        require(collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage + _teamperc1 + _teamperc2 == 100, "Change percentages");
        uint256 royalties = collectionTotalAmount[_collectionID];
        ...
        artistRoyalties1 = royalties * collectionArtistPrimaryAddresses[colId].add1Percentage / 100;
        artistRoyalties2 = royalties * collectionArtistPrimaryAddresses[colId].add2Percentage / 100;
        artistRoyalties3 = royalties * collectionArtistPrimaryAddresses[colId].add3Percentage / 100;
        // @audit team royalties can be manipulated 
        teamRoyalties1 = royalties * _teamperc1 / 100;
        teamRoyalties2 = royalties * _teamperc2 / 100;
        ...
        emit PayTeam(tm2, success5, teamRoyalties2);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Modify payArtist(...) to include checks to ensure this missing checks mentioned above are implemented as shown below

function payArtist(uint256 _collectionID, address _team1, address _team2, uint256 _teamperc1, uint256 _teamperc2) public FunctionAdminRequired(this.payArtist.selector) {
        require(collectionArtistPrimaryAddresses[_collectionID].status == true, "Accept Royalties");
        require(collectionTotalAmount[_collectionID] > 0, "Collection Balance must be grater than 0");
        require(collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage + _teamperc1 + _teamperc2 == 100, "Change percentages");
        require(add1Percentage + add2Percentage + add3Percentage == collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage, "Invalid Artist percentages");
        require(_teamperc1 + _teamperc2 == collectionRoyaltiesPrimarySplits[_collectionID].teamPercentage, "Invalid team percentages");
        uint256 royalties = collectionTotalAmount[_collectionID];
        ...
        artistRoyalties1 = royalties * collectionArtistPrimaryAddresses[colId].add1Percentage / 100;
        artistRoyalties2 = royalties * collectionArtistPrimaryAddresses[colId].add2Percentage / 100;
        artistRoyalties3 = royalties * collectionArtistPrimaryAddresses[colId].add3Percentage / 100;
        // @audit team royalties can be manipulated 
        teamRoyalties1 = royalties * _teamperc1 / 100;
        teamRoyalties2 = royalties * _teamperc2 / 100;
        ...
        emit PayTeam(tm2, success5, teamRoyalties2);
    }

Assessed type

Invalid Validation

c4-pre-sort commented 7 months ago

141345 marked the issue as duplicate of #703

c4-pre-sort commented 7 months ago

141345 marked the issue as not a duplicate

c4-pre-sort commented 7 months ago

141345 marked the issue as primary issue

c4-sponsor commented 7 months ago

a2rocket (sponsor) disputed

a2rocket commented 7 months ago

the check is done when the artist proposes addresses and percentages. The could no't exceed what was set by the admin.

c4-pre-sort commented 7 months ago

141345 marked the issue as sufficient quality report

alex-ppg commented 7 months ago

The Warden specifies that the percentages are not properly evaluated to sum to the expected values during the payArtist function.

The Sponsor states that these are indeed validated when set, however, an attack vector does exist due to a proposal / accepted discrepancy as explained in #1686.

The Warden has failed to identify this attack vector and simply states that an error may be present which I consider insufficient proof.

c4-judge commented 7 months ago

alex-ppg marked the issue as unsatisfactory: Insufficient proof