User is taken less fees when selling tokens in batch

[c4-submissions]

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

174:     function sell(uint256 _id, uint256 _amount) external {
175:         (uint256 price, uint256 fee) = getSellPrice(_id, _amount);
176:         // Split the fee among holder, creator and platform
177:         _splitFees(_id, fee, shareData[_id].tokensInCirculation);
178:         // The user also gets the rewards of his own sale (which is not the case for buys)
179:         uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
180:         rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
181: 
182:         shareData[_id].tokenCount -= _amount;
183:         shareData[_id].tokensInCirculation -= _amount;
184:         tokensByAddress[_id][msg.sender] -= _amount; // Would underflow if user did not have enough tokens
185: 
186:         // Send the funds to the user
187:         SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim + price - fee);
188:         emit SharesSold(_id, msg.sender, _amount, price, fee);
189:     }

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:

// The user also gets the rewards of his own sale (which is not the case for buys)

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 among N-1 tokens, because the previous operation already reduced tokensByAddress, then N-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 and shareId2 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, in share1 Alice purchases 10 tokens and then sells them in batch by calling market.sell(shareId1, 10). After the operation, she is left with 993384333333333343 payment tokens.
Charlie, purchases the same amount of tokens from share2, but he does 10 calls to market.sell(shareId2, 1), yielding 993188166666666670. Now, both have purchased the same amount of tokens and sold the same amount of tokens from shares that are identical, but Alice has 196166666666673 more than Charlie.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_Sell_FeeDifferenceInBatch() public {
    market.changeBondingCurveAllowed(address(bondingCurve), true);
    market.restrictShareCreation(false);

    // Create two shares
    vm.prank(bob);
    uint256 shareId1 = market.createNewShare("Test Share 1", address(bondingCurve), "metadataURI");
    vm.prank(bob);
    uint256 shareId2 = market.createNewShare("Test Share 2", address(bondingCurve), "metadataURI");

    // setup alice, charlie and mary with 1e18 tokens each
    uint256 initialAmount = 1e18;
    deal(address(token), alice, initialAmount);
    deal(address(token), charlie, initialAmount);
    deal(address(token), mary, initialAmount);

    // Mary buys some initial tokens on both shares
    vm.startPrank(mary);

    token.approve(address(market), type(uint256).max);

    market.buy(shareId1, 5);
    market.buy(shareId2, 5);

    vm.stopPrank();

    // Alice sells 10 tokens in one call
    vm.startPrank(alice);

    token.approve(address(market), type(uint256).max);

    market.buy(shareId1, 10);
    market.sell(shareId1, 10);

    market.claimHolderFee(shareId1);

    uint256 aliceFinalBalance = token.balanceOf(alice);
    console.log("Alice balance:", aliceFinalBalance);

    vm.stopPrank();

    // Charlie does 10 sells of 1 token
    vm.startPrank(charlie);

    token.approve(address(market), type(uint256).max);

    market.buy(shareId2, 10);

    for (uint256 index = 0; index < 10; index++) {
        market.sell(shareId2, 1);
    }

    market.claimHolderFee(shareId2);

    uint256 charlieFinalBalance = token.balanceOf(charlie);
    console.log("Charlie balance:", charlieFinalBalance);

    vm.stopPrank();

    // Alice got more after selling the same amount
    uint256 difference = aliceFinalBalance - charlieFinalBalance;
    assertGt(difference, 0);
    console.log("Difference:", difference);
}

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:

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.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[c4-sponsor]

OpenCoreCH (sponsor) acknowledged

[c4-sponsor]

OpenCoreCH marked the issue as disagree with severity

[OpenCoreCH]

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.

[MarioPoneder]

From PoC: 993384333333333343 / 993188166666666670 = 1.0001975
The resulting difference of fees in this example is 0.01975%. Therefore QA seems most appropriate.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-c

[romeroadrian]

@MarioPoneder I strongly think this issue classifies as med given the general consensus at c4. A couple of comments:

• I don't think the percentage difference shown in the PoC can be a reason to downgrade the issue. This just comes from the numbers shown in the PoC which are have been basically chosen randomly by me while crafting the test. Note that prices and fees have a configurable bonding curve that can be anything, these are not necessarily attached to the numbers I've set or even the default LinearBondingCurve.
• Issue Users will lose rewards when buying new tokens if they already own some tokens #9 shows a similar a case. How big are the missed fees there? Has this been analysed? In Users will lose rewards when buying new tokens if they already own some tokens #9 the test shown a difference of 66000000000000 while in the current report the difference 196166666666673. I believe this is being judged unfairly.

Thanks

[MarioPoneder]

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.

[romeroadrian]

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:

1. The issue mentioned in the README (NFT minting / burning fees) happens when the user mints the NFT for the shares (NOT buying or selling the actual shares). This issue is about the actual selling un bulk of shares (not NFT involved).
2. I do think Users will lose rewards when buying new tokens if they already own some tokens #9 is invalid due to it being intentionally, thus design choice (left a comment there). When buying, the fees are intentionally left aside. In the selling case is not, which raises the issue described in this report. If Users will lose rewards when buying new tokens if they already own some tokens #9 is considered valid, then I think there are even more reasons to have the current issue as a valid med.

[OpenCoreCH]

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.

[MarioPoneder]

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.

[c4-judge]

MarioPoneder marked the issue as grade-a

[romeroadrian]

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?