-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
xiaoming90 - Broken batch minting feature #280
Labels
Has Duplicates
A valid issue with 1+ other issues describing the same vulnerability
Medium
A valid Medium severity issue
Reward
A payout will be made for this issue
Sponsor Confirmed
The sponsor acknowledged this issue is valid
Will Fix
The sponsor confirmed this issue will be fixed
Comments
This was referenced May 6, 2024
I don't think the duplicates on this issue are actually duplicates of this issue. @Hash01011122 |
This was referenced May 12, 2024
This was referenced May 12, 2024
This was referenced May 13, 2024
This was referenced May 13, 2024
The protocol team fixed this issue in the following PRs/commits: |
The Lead Senior Watson signed off on the fix. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Has Duplicates
A valid issue with 1+ other issues describing the same vulnerability
Medium
A valid Medium severity issue
Reward
A payout will be made for this issue
Sponsor Confirmed
The sponsor acknowledged this issue is valid
Will Fix
The sponsor confirmed this issue will be fixed
xiaoming90
medium
Broken batch minting feature
Summary
The core minting feature of the protocol is broken due to the mishandling of
msg.value
within the for-loop.Vulnerability Detail
Assume that the total fee for each token is 0.001 ETH, and Bob wants to mint four tokens. The total fee will be 0.004 ETH, so he will send 0.004 ETH when calling the above
mintBatch
function.An important point to note is that the
msg.value
will always remain at 0.004 ETH throughout the entire execution of themintBatch
function. Themsg.value
will not automatically be reduced regardless of how many ETH has been transferred out or "spent".In the first for-loop, the
msg.value
will be 0.004 ETH, and all 0.004 ETH will be routed to the fee manager and subsequently routed to the fee recipient address/0xSplit wallet.In the second for-loop, since all the ETH (0.004 ETH) was sent to the fee manager earlier, the amount of ETH left on the Edition contract is zero. When the second for-loop attempts to send
msg.value
(0.004 ETH) to the fee manager again, it will revert due to insufficient ETH, and the transaction will fail and revert. Thus, this batch minting feature is broken.https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L277
Impact
Breaks core contract functionality. The batch minting feature, a core feature of the protocol, is broken.
Code Snippet
https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L277
Tool used
Manual Review
Recommendation
For each loop, consider only forwarding/transferring the minting fee for the current token ID instead of the entire ETH (
msg.value
).The text was updated successfully, but these errors were encountered: