Open c4-submissions opened 10 months ago
minhquanym marked the issue as sufficient quality report
OpenCoreCH (sponsor) acknowledged
OpenCoreCH marked the issue as disagree with severity
True, but I do not think that this is an issue in practice. This is favourable for the user (when a user wants to sell N tokens now, they typically would do this in one transaction and not N, unless doing it in N would be favourable). Moreover, the difference is pretty small (for reasonable amounts) and avoiding it would require relatively involved calculations for batches.
From PoC: 993384333333333343 / 993188166666666670 = 1.0001975
The resulting difference of fees in this example is 0.01975%
. Therefore QA seems most appropriate.
MarioPoneder changed the severity to QA (Quality Assurance)
MarioPoneder marked the issue as grade-c
@MarioPoneder I strongly think this issue classifies as med given the general consensus at c4. A couple of comments:
66000000000000
while in the current report the difference 196166666666673
. I believe this is being judged unfairly. Thanks
Thank you for your comment!
After having a second look (from report):
When tokens are sold, a percentage of the fees go to each token holder, including the seller. If a seller sells tokens in batch, they will get more in fees since each one of the tokens being sold is still counted as a token the user holds.
and
Although similar, this is a different scenario than the one mentioned in the contest documentation:
NFT minting / burning fees are based on the current supply. This leads to the situation that buying 100 tokens and then minting 100 NFTs is more expensive than buying 1, minting 1, buying 1, minting 1, etc... (100 times). We do not consider this a problem because a user typically has no incentives to mint more than one NFT.
It's not clear how this is a different scenario like pointed out by the Publicly Known Issues
in the README.
@OpenCoreCH could you please have a second look?
Furthermore, #9 leads to small user loss while the present issue leads to small user gain (less fess) which seems to be an accepted design choice by the protocol.
Thank you for your comment!
After having a second look (from report):
When tokens are sold, a percentage of the fees go to each token holder, including the seller. If a seller sells tokens in batch, they will get more in fees since each one of the tokens being sold is still counted as a token the user holds.
and
Although similar, this is a different scenario than the one mentioned in the contest documentation:
NFT minting / burning fees are based on the current supply. This leads to the situation that buying 100 tokens and then minting 100 NFTs is more expensive than buying 1, minting 1, buying 1, minting 1, etc... (100 times). We do not consider this a problem because a user typically has no incentives to mint more than one NFT.
It's not clear how this is a different scenario like pointed out by the
Publicly Known Issues
in the README. @OpenCoreCH could you please have a second look?Furthermore, #9 leads to small user loss while the present issue leads to small user gain (less fess) which seems to be an accepted design choice by the protocol.
Right, let's separate concerns here:
Thank you for your comment!
After having a second look (from report):
When tokens are sold, a percentage of the fees go to each token holder, including the seller. If a seller sells tokens in batch, they will get more in fees since each one of the tokens being sold is still counted as a token the user holds.
and
Although similar, this is a different scenario than the one mentioned in the contest documentation:
NFT minting / burning fees are based on the current supply. This leads to the situation that buying 100 tokens and then minting 100 NFTs is more expensive than buying 1, minting 1, buying 1, minting 1, etc... (100 times). We do not consider this a problem because a user typically has no incentives to mint more than one NFT.
It's not clear how this is a different scenario like pointed out by the
Publicly Known Issues
in the README. @OpenCoreCH could you please have a second look?Furthermore, #9 leads to small user loss while the present issue leads to small user gain (less fess) which seems to be an accepted design choice by the protocol.
I agree that this is a different scenario than the one mentioned in the contest description. The description was about mintNFT
& burnNFT
, this is about sell
. The difference is acceptable to us in both scenarios, but only one was pointed out in the description.
Given the fact that this is intended design like in case of the similar issue as pointed out in the README, QA will be maintained.
MarioPoneder marked the issue as grade-a
Given the fact that this is intended design like in case of the similar issue as pointed out in the README, QA will be maintained.
Mario, sorry I really think there is an inconsistent judgement here.
Your first argument to have this downgraded was a small difference in the taken fees, which I shown it was inconsistent with #9 since my report demonstrated higher differences.
Now the argument is an intended design, which is clearly NOT the case, even stated by the sponsor in the last comment. Even if so it contradicts again with #9, which is indeed intended design and is being judged as medium.
This isn't making sense, can you please review the situation?
Lines of code
https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L174-L189
Vulnerability details
Summary
When tokens are sold, a percentage of the fees go to each token holder, including the seller. If a seller sells tokens in batch, they will get more in fees since each one of the tokens being sold is still counted as a token the user holds.
Impact
By design, tokens sold in the 1155tech Market distribute fees to token holders, including the same holder who is selling the tokens. The behavior is present in the
sell()
function:https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L174-L189
Line 177 splits the fees, which distribute the holder share of the fee between all the token holders (fees are divided by
tokensInCirculation
). Seller tokens are later decremented in line 184. This means that fees generated from the sell also apply to the seller, which is stated by the comment in line 178:However, this implementation contains an issue where a seller that sells tokens in batch will get more fees than a seller who sells one by one, or in smaller chunks.
When a shareholder sells N tokens, they will benefit from collecting fees from all N tokens. This happens because the operation is done in batch and the fees will be distributed before the N tokens are reduced from
tokensByAddress
. When the same operation is done one by one, the first sell will be distributed between N tokens, but the next sell will be distributed amongN-1
tokens, because the previous operation already reducedtokensByAddress
, thenN-2
, and so on.Proof of Concept
We demonstrate the issue in the following test. We mock two identical shares to show the issue as a difference between both scenarios, a seller who sells in batch and a seller who sells the same tokens, at the same time, at the same prices, but one by one.
shareId1
andshareId2
are two different shares with the same parameters and bonding curve. First we bootstrap both shares by making Mary buy the same amount of shares in each one. Then, inshare1
Alice purchases 10 tokens and then sells them in batch by callingmarket.sell(shareId1, 10)
. After the operation, she is left with993384333333333343
payment tokens. Charlie, purchases the same amount of tokens fromshare2
, but he does 10 calls tomarket.sell(shareId2, 1)
, yielding993188166666666670
. Now, both have purchased the same amount of tokens and sold the same amount of tokens from shares that are identical, but Alice has196166666666673
more than Charlie.Note: the snippet shows only the relevant code for the test. Full test file can be found here.
Recommendation
Selling tokens in batch should yield the same fees as selling them one by one or in smaller batches. When selling more than 1 token, the fees from each of the tokens being sold need to account for other tokens sold in the batch, so that fees are properly accounted for in both cases.
Note from warden
Although similar, this is a different scenario than the one mentioned in the contest documentation:
Assessed type
Other