Canto Application Specific Dollars and Bonding Curves for 1155s - leegh's results

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/Market.sol#L152-L159

Vulnerability details

Impact

If a user already haves tokens in a share, when he buy for that share again, part of his fee will be stucked in the market contract forever.

Proof of Concept

When a user makes a buy transaction for a share, he needs to pay a fee. The fee is splitted among token holders, share creator and the platform through _splitFees. The part splitted to token holders will be transferred to other users proportioned to their tokens.

File: 1155tech-contracts/src/Market.sol, Function: buy()

152:        (uint256 price, uint256 fee) = getBuyPrice(_id, _amount); // Reverts for non-existing ID
153:        SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee);
154:        // The reward calculation has to use the old rewards value (pre fee-split) to not include the fees of this buy
155:        // The rewardsLastClaimedValue then needs to be updated with the new value such that the user cannot claim fees of this buy
156:        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
157:        // Split the fee among holder, creator and platform
158:        _splitFees(_id, fee, shareData[_id].tokensInCirculation);  // @audit: holder fees is splitted to ALL tokens in circulation
159:        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
160:
161:        shareData[_id].tokenCount += _amount;
162:        shareData[_id].tokensInCirculation += _amount;
163:        tokensByAddress[_id][msg.sender] += _amount;
164:
165:        if (rewardsSinceLastClaim > 0) {
166:            SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim);
167:        }

https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/Market.sol#L152-L167

A user is not allowed to get fees for his own buyings (L155). But his buy fee (the part for token holders) is splitted to ALL tokens in circulation (L158), including his own existing tokens. The part fee for his existing tokens will not be able to be withdrawn and will be stucked in the market contract forever.

Let's see it in more details with a test case.

// Add a new test case in 1155tech-contracts/src/Market.t.sol.

function testBuySell() public {
    testCreateNewShare();
    token.approve(address(market), 10 * 1e18);

    address holder = address(this);
    address creator = bob;
    address platform = address(market);
    
    console.log("Initial: holder=%d, creator=%d, platform=%d", token.balanceOf(holder), 
                token.balanceOf(creator), token.balanceOf(platform));

    // 1. buy 3 tokens, fee incurs
    market.buy(1, 1);
    market.buy(1, 1);
    market.buy(1, 1);
    console.log("After buy: holder=%d, creator=%d, platform=%d", token.balanceOf(holder), 
                token.balanceOf(creator), token.balanceOf(platform));

    // 2. sell all the tokens, and claim all fee if have
    market.sell(1, 3);
    market.claimHolderFee(1);

    // 3. creator claims creator fee
    vm.prank(creator);
    market.claimCreatorFee(1);

    // 4. owner claims platform fee
    vm.prank(address(this));
    market.claimPlatformFee();

    console.log("Ending: holder=%d, creator=%d, platform=%d", token.balanceOf(holder), 
                token.balanceOf(creator), token.balanceOf(platform));
    
    // 5. some tokens still remains in the platform
    assertNotEq(token.balanceOf(platform), 0);
}

Run the test case with forge test -vvvv --match-test testBuySell, and the result log shows as follows. We can see that the ending balance of platform (i.e. market contract) is NOT 0.

Logs:
  setUp balance: market=0, user=1000000000000000000000
  Initial: holder=1000000000000000000000, creator=0, platform=0
  After buy: holder=999993400000000000000, creator=0, platform=6600000000000000
  Ending: holder=999999439000000000000, creator=396000000000000, platform=165000000000000

Tools Used

Manual audit

Recommended Mitigation Steps

When splitting fees, subtract the amount of the caller's token from tokensInCirculation.

@@ -155,7 +155,7 @@ contract Market is ERC1155, Ownable2Step {
         // The rewardsLastClaimedValue then needs to be updated with the new value such that the user cannot claim fees of this buy
         uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
         // Split the fee among holder, creator and platform
-        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
+        _splitFees(_id, fee, shareData[_id].tokensInCirculation - tokensByAddress[_id][msg.sender]);
         rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
 
         shareData[_id].tokenCount += _amount;

Assessed type

Token-Transfer