Open c4-submissions opened 9 months ago
141345 marked the issue as sufficient quality report
141345 marked the issue as duplicate of #303
alex-ppg marked the issue as not a duplicate
alex-ppg marked the issue as primary issue
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.
alex-ppg marked the issue as selected for report
alex-ppg marked the issue as satisfactory
Hi @alex-ppg, here is why I believe this issue in not valid:
setPrimaryAndSecondarySplits() => proposePrimaryAddressesAndPercentages() => acceptAddressesAndPercentages
.Thank you for taking the time to read this.
Hey @mcgrathcoutinho, thanks for contributing to this particular submission's PJQA process! Let me get back to each of your points:
The issue assumes that the artist is willing to break the flow which is a valid assessment.
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.
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.
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.
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
: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
:Then, also when paid out, there is a validation that the split between team and artist allocation adds up:
MinterContract::payArtist
: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
:Hence, there is a possibility that up to double the intended amount can be paid out, imagine this scenario:
They then call
payArtist
with the 100% team allocation. This will pass the check asartistPercentage
will be0
. 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 needsforge-std
inhardhat/lib
: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