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

Lines of code

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

Vulnerability details

Impact

When users purchase new tokens, the old tokens they already own do not receive the fees from this purchase, resulting in a loss of funds for the users.

Proof of Concept

According to the explanation by @Roman, when users buy new tokens, the recently purchased token is not eligible for a fee. However, the "old shares" that users already own should be eligible.

Lambda: Hey, just a design decision, the idea behind it is that the user does not own the new shares yet when he buys, so he should not get any fees for it. Whereas he still owns it when he sells (it is basically the last sale)

Krace: So the new shares should not get fee. If the user owns some "old shares", is it eligible for a fee when the user buying?

Lambda: Yes, in that case, these shares are in the "owner pool" at the time of purchase and should be eligible

However, the buy function will completely disregard the rewards associated with the buyer's "old shares" for these fees.

Let's analyze the buy function:

• Step 1: It retrieves the rewards for the buyer before the purchase. These rewards are not linked to the purchase because they use the old token amounts, and the fees from this purchase are not factored in.
• Step 2: It splits the fees for this purchase. _splitFees increases the value of shareHolderRewardsPerTokenScaled based on the fees from this purchase.
• Step 3: It updates the rewardsLastClaimedValue of the buyer to the latest shareHolderRewardsPerTokenScaled, which was increased in _splitFees just now.

The question arises: the fees from this purchase are not rewarded to the buyer's old tokens between Step 2 and Step 3, yet the buyer's rewardsLastClaimedValue is updated to the newest value. This results in the permanent loss of these rewards for the buyer.

    function buy(uint256 _id, uint256 _amount) external {
        require(shareData[_id].creator != msg.sender, "Creator cannot buy");
        (uint256 price, uint256 fee) = getBuyPrice(_id, _amount); // Reverts for non-existing ID
        SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee);
        // The reward calculation has to use the old rewards value (pre fee-split) to not include the fees of this buy
        // The rewardsLastClaimedValue then needs to be updated with the new value such that the user cannot claim fees of this buy
//@audit it will get the rewards first        
        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        // Split the fee among holder, creator and platform
//@audit then split the fees of this buying        
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
//@audit and update sender's rewardsLastClaimedValue to the newest one, which is updated by `splitFees`
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

        shareData[_id].tokenCount += _amount;
        shareData[_id].tokensInCirculation += _amount;
        tokensByAddress[_id][msg.sender] += _amount;

        if (rewardsSinceLastClaim > 0) {
            SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim);
        }
        emit SharesBought(_id, msg.sender, _amount, price, fee);
    }

    function _splitFees(
        uint256 _id,
        uint256 _fee,
        uint256 _tokenCount
    ) internal {
        uint256 shareHolderFee = (_fee * HOLDER_CUT_BPS) / 10_000;
        uint256 shareCreatorFee = (_fee * CREATOR_CUT_BPS) / 10_000;
        uint256 platformFee = _fee - shareHolderFee - shareCreatorFee;
        shareData[_id].shareCreatorPool += shareCreatorFee;
        if (_tokenCount > 0) {
//@audit shareHolderRewardsPerTokenScaled will be increased if tokenCount>0
            shareData[_id].shareHolderRewardsPerTokenScaled += (shareHolderFee * 1e18) / _tokenCount;
        } else {
            // If there are no tokens in circulation, the fee goes to the platform
            platformFee += shareHolderFee;
        }
        platformPool += platformFee;
    }

Test

The following poc shows the case mentioned before, add it to 1155tech-contracts/src/Market.t.sol and run it with forge test --match-test testBuyNoReward -vv

    function testBuyNoReward() public {
        // Bob creat share with id 1
        market.changeBondingCurveAllowed(address(bondingCurve), true);
        market.restrictShareCreation(false);
        vm.prank(bob);
        market.createNewShare("Test Share", address(bondingCurve), "metadataURI");
        assertEq(market.shareIDs("Test Share"), 1);

        token.transfer(alice, 1e17);

        // alice buy 1 token in share1 ==> token1
        vm.startPrank(alice);
        uint256 aliceBalanceBefore = token.balanceOf(alice);
        token.approve(address(market), 1e18);
        console.log(" alice balance ", aliceBalanceBefore);
        market.buy(1, 1);
        uint256 aliceBalanceAfter = token.balanceOf(alice);
        console.log(" alice balance after buy ",  aliceBalanceAfter);
        console.log(" alice cost after buy ",  aliceBalanceBefore - aliceBalanceAfter);

        // alice buy another 1 token in share1 ==> token2
        market.buy(1,1);
        uint256 aliceBalanceAfterSecondBuy = token.balanceOf(alice);
        console.log(" alice balance after second buy ",  aliceBalanceAfterSecondBuy);
        // alice get no reward for her token1
        console.log(" alice cost after second buy ",  aliceBalanceAfter - aliceBalanceAfterSecondBuy);

        vm.stopPrank();
    }

The result is shown below, alice gets no rewards for token1

[PASS] testBuyNoReward() (gas: 443199)
Logs:
   alice balance  100000000000000000
   alice balance after buy  98900000000000000
   alice cost after buy  1100000000000000
   alice balance after second buy  96700000000000000
   alice cost after second buy  2200000000000000

After my suggested patch, alice can get the rewards for token1 : 2200000000000000 - 2134000000000000 = 66000000000000

[PASS] testBuyNoReward() (gas: 447144)
Logs:
   alice balance  100000000000000000
   alice balance after buy  98900000000000000
   alice cost after buy  1100000000000000
   alice balance after second buy  96766000000000000
   alice cost after second buy  2134000000000000

Tools Used

Foundry

Recommended Mitigation Steps

Split the fees before getting the rewards in buy:

diff --git a/1155tech-contracts/src/Market.sol b/1155tech-contracts/src/Market.sol
index 59c5c96..85d91a5 100644
--- a/1155tech-contracts/src/Market.sol
+++ b/1155tech-contracts/src/Market.sol
@@ -151,11 +151,11 @@ contract Market is ERC1155, Ownable2Step {
         require(shareData[_id].creator != msg.sender, "Creator cannot buy");
         (uint256 price, uint256 fee) = getBuyPrice(_id, _amount); // Reverts for non-existing ID
         SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee);
+        // Split the fee among holder, creator and platform
+        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
         // The reward calculation has to use the old rewards value (pre fee-split) to not include the fees of this buy
         // 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);
         rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

         shareData[_id].tokenCount += _amount;

Assessed type

Context