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

Lines of code

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

Vulnerability details

Proof of Concept

Price for shares inside Market is calculated using bonding curve. Currently LinearBondingCurve is supported. This bonging curve increases each next shares with fixed amount and also uses 10% / log2(shareIndex) to calculate fee for the share.

In order to calculate price to buy shares getBuyPrice is used and to calculate price to sell shares getSellPrice is used.

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

    function getBuyPrice(uint256 _id, uint256 _amount) public view returns (uint256 price, uint256 fee) {
        // If id does not exist, this will return address(0), causing a revert in the next line
        address bondingCurve = shareData[_id].bondingCurve;
        (price, fee) = IBondingCurve(bondingCurve).getPriceAndFee(shareData[_id].tokenCount + 1, _amount);
    }

    function getSellPrice(uint256 _id, uint256 _amount) public view returns (uint256 price, uint256 fee) {
        // If id does not exist, this will return address(0), causing a revert in the next line
        address bondingCurve = shareData[_id].bondingCurve;
        (price, fee) = IBondingCurve(bondingCurve).getPriceAndFee(shareData[_id].tokenCount - _amount + 1, _amount);
    }

So both functions use IBondingCurve(bondingCurve).getPriceAndFee, but getBuyPrice provides current token index as start index for curve, while getSellPrice provides current token index - amount to sell as start index for curve.

So in case when someone wants to buy shares, then price depends on current circulation supply. In case if this supply is increased right after user's buy tx, then he will pay more for the shares. And in case if someone sells shares right before buy tx, then user will pay less amount(which is good of course).

The same we can say about sell function. In case if someone sells shares right before user's sell execution, then user receives smaller amount and in case if someone buys shares, then user gets bigger amount.

As the price at the moment when user initiates buy or sell function can be changed before execution(with frontrunning or just innocent), that means that slippage protection should be introduced, so user can be sure that he will not pay more than expected or receive less than expected.

When user expect to buy share for 5 usd, then attacker can sandwhich user's tx shares in order to make profit.

• attacker first frontruns and buy shares that cost 5 usd
• then let user's buy tx to be executed so user buys shares more expensive(10 usd)
• then attacker backrun with sell shares that cost now 10 usd and earn on this(5 usd).

Same thing can be done with sell sandwhiching. When user expects to sell his share for 20 usd

• attacker can sell own share(and get 20 usd)
• then let user sell his share cheaper(and get 15 usd)
• then attacker buys share cheaper(for 10 usd) and earn on this(10$).

While buy sandwhiching needs user to give full approval to Market as this price is then sent from user, sell sandwhiching doesn't need that, because it only sends tokens to the victim.

Impact

User can lose funds.

Tools Used

VsCode

Recommended Mitigation Steps

Make user provide slippage to buy, sell, burnNFT and mintNFT functions.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

When user buys shares, then their's price and fee is calculated. They are paid by user. And fees are split among protocol creator, share creator and shares holders.

Fee split is done using _splitFees. This function increases shareData[_id].shareHolderRewardsPerTokenScaled, as fee is distributed among shares. The function has _tokenCount param, which in our case is passed as all tokens in circulation.

Because _splitFees changes shareData[_id].shareHolderRewardsPerTokenScaled user's accumulated reward is calculated before _splitFees is called. After fee split, user's rewardsLastClaimedValue[_id][msg.sender] variable is updated to the current shareData[_id].shareHolderRewardsPerTokenScaled, which means that user is fully settled with rewards at the moment.

But this can be not like that. In case if user had shares before buy call, then it means that amount of shares that he already has are included into shareData[_id].tokensInCirculation, which means that fee that he provided in current buy call is also distributed to his old shares. But because rewardsLastClaimedValue[_id][msg.sender] is reset for user to the new value, that means that those rewards will not be claimable by him and they will stay in the contract.

Impact

User losses part ofrewards

Tools Used

VsCode

Recommended Mitigation Steps

Call uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id) after split is done, so new reward rate is used for old shares.

        - uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        // Split the fee among holder, creator and platform
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        + uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

Assessed type

Error