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

Lines of code

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

Vulnerability details

Impact

If another user frontruns a buy or sell transaction, the order may be executed at an unintended price.

Proof of Concept

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
        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;
        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);
    }

The buy function receives tokens from the user through transferFrom. Therefore, users are likely to max approve their tokens to the market for use.

If a malicious user frontruns the buy, raising the price, the user ends up buying at a higher price than intended.

Afterwards, the malicious user can sell at a higher price than they bought for.

POC:

function testFrontrun() public {
        testBuy();
        vm.prank(vic);
        token.approve(address(market), type(uint256).max);

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

        uint256 before = token.balanceOf(bad);

        vm.prank(bad);
        market.buy(1, 1); // <- frontrun

        vm.prank(vic);
        market.buy(1, 1);

        vm.prank(bad);
        market.sell(1, 1); // <- and sell

        uint256 after = token.balanceOf(bad);
        console.log(after - before);
    }
// console : 582500000000000

Tools Used

VS Code

Recommended Mitigation Steps

The buy and sell functions should take a minimum price value as an argument for a slippage check.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L147-L169

Vulnerability details

Impact

A portion of the holder share from the amount purchased through 'buy' is locked in the contract.

Proof of Concept

/// @notice Buy amount of tokens for a given share ID
    /// @param _id ID of the share
    /// @param _amount Amount of shares to buy
    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
        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;
        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);
    }

Through the Market.buy function, one can purchase shares created by a specific creator.

The user attempting to purchase must pay the token price + fee, and this fee is divided among the holder, creator, and platform.

The buyer cannot distribute the currently purchased share.

However, this amount is not distributed to the creator, creator, or platform, but rather is locked in the contract.

The following scenario occurs:

1. Alice buys 1 share.
2. Since it's the first share, there is no holder distribution.
3. Alice buys 1 share.
4. rewardsSinceLastClaim is set to 0, thus no holder distribution.
5. splitFees is called. Since Alice has a share from the first purchase, the holder fee is distributed to Alice.
6. shareHolderRewardsPerTokenScaled is updated.
7. Alice's rewardsLastClaimedValue is updated to shareHolderRewardsPerTokenScaled (unable to claim the fee distributed in step 5).

POC

function testclaimPlatform() public {
        testBuy();
        market.claimPlatformFee();
        for(uint i =0;i<1000;i++)
            market.buy(1,1);

        market.claimPlatformFee();
        
        market.sell(1, 1001);

        market.claimPlatformFee();

        vm.prank(bob);
        market.claimCreatorFee(1);

        console.log(token.balanceOf(address(market)));
    }
// console: 1928000904761904273 (1.928e18)

Tools Used

VS Code

Recommended Mitigation Steps

    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

        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        // Split the fee among holder, creator and platform
        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);
    }

Do _splitFees first. Since the tokenCount hasn't increased, the inability to split fees for ongoing buys remains.

Assessed type

Other