Closed sherlock-admin3 closed 4 months ago
Escalate
This is a valid medium issue as there is a clear economical disincentive in the code. Kindly ask the sponsor to confirm.
Escalate
This is a valid medium issue as there is a clear economical disincentive in the code. Kindly ask the sponsor to confirm.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
This is not a vulnerability, this is just economic incentives (only present at certain configurations + low price threshold) that we took into account while designing and are okay with.
Here is the exact problem, prices from 0.0006 to 0.0010 will never be picked.
It is a security issue when it causes some creators to not be able to pick the prices they want at and receive fees accordingly. Some creator that tries to sell at a price of 0.0007 will receive very small fees compared to a price of 0.0006, so in practice he won't be able to pick this price (assuming he's acting in his best interests).
Thus, creators are essentially DoSed of picking a certain price range (0.0006 to 0.0010).
Understood, although this is not a security issue. This could be marked as informational.
Not commenting about your opinion @pqseags, just leaving here a relevant part of the docs
https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue
Breaks core contract functionality
Essentially not being able to mint in a certain price range falls in this scope.
I disagree that this breaks core contract functionality. Core contract functionality is to publish works, attribute references, and earn proceeds. This is still possible and there is no loss of funds. This just disincentivizes somebody to publish at a certain price point. We were aware of this nuance during the design, so it certainly does not break contract functionality.
I appreciate your input, and will leave it to the judges to assess this further.
For future reference there is a section in the readme about known issues.
Please list any known issues/acceptable risks that should not result in a valid finding. N/A
I believe this calculations are indeed is the design decision and works as it should be. Planning to reject the escalation and leave the issue as it is.
@WangSecurity just adding the fact that the sponsor agrees it is an issue. I don't think it's up to him to decide if the issue is a security one or the exact classification is informational.
This is not a vulnerability, this is just economic incentives (only present at certain configurations + low price threshold) that we took into account while designing and are okay with. Understood, although this is not a security issue. This could be marked as informational.
This is an issue that the sponsor is okay with because they designed the mechanism and then figured out that this happens. In this case, it should have gone to the known risks section or it is valid. The rules should be more clear when such situations happen. Ultimately it is up to the judge so I respect your decision.
I understand your thoughts and, of course, my decision is not based on the words by the comment from sponsor. But, I see this as a design decision, I agree that the current formula is not as good for the protocol as it could be, but I believe this is actually a recommendation to improve the current accounting system. Hence I believe it should remain invalid.
Planning to reject the escalation and leave the issue as it is.
@WangSecurity I am just adding additional facts to the judgement of this issue.
This finding is exactly the same as this one, incorrect economic incentives.
Thank you for that reference, but historical decisions are not sources of truth and this two findings have a different contest and I disagree can be called the same. Hence, the decision remains the same, reject the escalation and leave the issue as it is.
Result: Invalid Unique
0x73696d616f
medium
Inconsistent
protocolShare
calculation inFeeManager::_collectMintFee()
that will likely incentivize creators to pick lower mint feesSummary
protocolShare
calculation inFeeManager::_collectMintFee()
can lead to more fees for thecreator
if themintFee
decreases, incentivizing smaller mint fees for the loss of the protocol.Vulnerability Detail
FeeManager::_collectMintFee()
calculates theprotocolShare
by doing:Using the numbers hardcoded in the contract,
protocolFlatFee = 0.0006 ether
fee_.amount = amount * (mintFee + protocolFlatFee)
protocolFeeshareBps = 3333
creatorFee = fee_.amount - protocolShare
Now, consider an amount of 1.If
mintFee == 0
, protocol receivesprotocolFee * protocolFeeshareBps / MAX_BPS; = protocolFlatFee * amount * protocolFeeshareBps / MAX_BPS = 0.0006 * 1 * 1 / 3 = 0.0002 ether
creator receives0.0006 - 0.0002 = 0.0004 ether
if
mintFee == 1
, protocol receivesprotocolFlatFee*amount = 0.0006
creator receivesamount * (1 + 0.0006 ether) - 0.0006 ether = 1 * (1 + 0.0006 ether) - 0.0006 ether = 1
Therefore, in order to receive the same amount of fees, the creator would have to charge a fee of
0.004
(1 * (0.004 + 0.0006) - 0.006 = 0.004 ether
.Taking into account price and demand, it should be much more profitable for the creator to pick the lowest price here (free mint), as the minter would pay only
0.0006 ether
, much less than0.0010
, for the same fees for the creator.Impact
Loss of yield from the protocol as creators are incentivized to pick lower prices.
Code Snippet
https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L366
Tool used
Manual Review
Vscode
Recommendation
The fees for the creator, in case
fee.amount > protocolFee
, should increase continuously in comparison tofee.amount == protocolFee
, or the protocol risks losing yield due to mismatched incentives.