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

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/asD/src/asD.sol#L73-L78

Vulnerability details

Vulnerability details

The asD.withdrawCarry() method is mainly used to withdraw profits. An important step in this process is the calculation of maximumWithdrawable. This variable is used to prevent the owner from withdrawing the note deposited by the user, ensuring a 1:1 exchange. The calculation formula for maximumWithdrawable is as follows:

    function withdrawCarry(uint256 _amount) external onlyOwner {
        uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent(); // Scaled by 1 * 10^(18 - 8 + Underlying Token Decimals), i.e. 10^(28) in our case
        // The amount of cNOTE the contract has to hold (based on the current exchange rate which is always increasing) such that it is always possible to receive 1 NOTE when burning 1 asD
        uint256 maximumWithdrawable = (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) /
@>          1e28 -
            totalSupply();
        if (_amount == 0) {
            _amount = maximumWithdrawable;
        } else {
            require(_amount <= maximumWithdrawable, "Too many tokens requested");
        }

Currently, it is assumed that the number of digits is 28, which is actually misleading in the documentation. Let's see how many digits there actually are. Online address: https://tuber.build/address/0xEe602429Ef7eCe0a13e4FfE8dBC16e101049504C?tab=read_contract block:6896358 exchangeRateCurrent = 1004019984703570700 = 1.004e18

Here we can see the official explanation of this document. Currently, cNote should be 1e18. https://github.com/compound-finance/compound-protocol/pull/167

This isn't quite right and also we wouldn't change the comments on already deployed contracts. But to explain: the exchange rate is somewhat arbitrary, and although its interpretation will depend on the decimals of the cToken and the decimals of the underlying, once you have a desired initial exchange rate, the mantissa is simply scaled by 1e18.

Impact

The incorrect use of 28 digits for calculation causes maximumWithdrawable to be far less than the actual value making it impossible to withdraw profits, and the funds are locked in the contract.

Recommended Mitigation

    function withdrawCarry(uint256 _amount) external onlyOwner {
        uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent(); // Scaled by 1 * 10^(18 - 8 + Underlying Token Decimals), i.e. 10^(28) in our case
        // The amount of cNOTE the contract has to hold (based on the current exchange rate which is always increasing) such that it is always possible to receive 1 NOTE when burning 1 asD
        uint256 maximumWithdrawable = (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) /
-           1e28 -
+           1e18 -
            totalSupply();
        if (_amount == 0) {
            _amount = maximumWithdrawable;
        } else {
            require(_amount <= maximumWithdrawable, "Too many tokens requested");
        }

Assessed type

Decimal

Lines of code

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

Vulnerability details

Vulnerability details

in market.sol Whether it's buy(),sell(), or mintNFT(), the price is calculated in real-time. It is affected by shareData[id].tokenCount. The larger the tokenCount, the more expensive it is, and the smaller the tokenCount, the cheaper it is.

    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
    function getBuyPrice(uint256 _id, uint256 _amount) public view returns (uint256 price, uint256 fee) {
...
@>      (price, fee) = IBondingCurve(bondingCurve).getPriceAndFee(shareData[_id].tokenCount + 1, _amount);
    }

contract LinearBondingCurve is IBondingCurve {
    // By how much the price increases per share, provided in the token decimals
    uint256 public immutable priceIncrease;

    constructor(uint256 _priceIncrease) {
        priceIncrease = _priceIncrease;
    }

    function getPriceAndFee(uint256 shareCount, uint256 amount)
        external
        view
        override
        returns (uint256 price, uint256 fee)
    {
        for (uint256 i = shareCount; i < shareCount + amount; i++) {
@>          uint256 tokenPrice = priceIncrease * i;
            price += tokenPrice;
            fee += (getFee(i) * tokenPrice) / 1e18;
        }
    }

Currently, buy() and sell() only pass in two parameters: _id, amount.

This a problem. The price that users see on the front-end UI may be much different by the time the transaction is executed.

For example: When the user sees it on the UI: tokenCount = 1000, price=10 After submitting the transaction, the price may have soared to 20 due to a whale buying a lot. After the transaction is executed, someone may have sold a lot, causing the price to drop back to 5.

In this case, users may pay a lot more, and may even suffer from sandwich attacks.

It is recommended to add maxPrice and minPrice similar to the slippage in swap().

Note: mintNFT() also has a similar problem.

Impact

The price may change without the user's expectation, leading to fund loss

Recommended Mitigation

It is recommended to add maxPrice and minPrice similar to the slippage in swap().

-   function buy(uint256 _id, uint256 _amount) external {
+   function buy(uint256 _id, uint256 _amount , uint256 maxPrice) external {
...

-   function sell(uint256 _id, uint256 _amount) external {
+   function sell(uint256 _id, uint256 _amount, uint256 minPrice) external {

Assessed type

MEV

Lines of code

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

Vulnerability details

Vulnerability details

In Market.buy(), when a user makes a purchase, the user's rewards are settled and the current transaction fee is allocated. The steps are as follows:

1. Calculate the transaction fee fee by getBuyPrice()
2. Settle the current user's rewards _getRewardsSinceLastClaim()
3. Allocate the current transaction fee _splitFees()
4. Modify the user's rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled

The code is as follows:

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

Normally, the third step _splitFees() should be placed before the second step _getRewardsSinceLastClaim(). However, the protocol does not want to allocate this fee to the current user, so _getRewardsSinceLastClaim() is executed first. But the problem lies in the parameter passed to _splitFees(), _tokenCount = shareData[_id].tokensInCirculation, which may include the current user's share. This makes the denominator too large, and the allocated fee cannot be retrieved, because the user has already synchronized to the latest rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled in the fourth step.

For example: Currently, there are two users, alice and bob tokensByAddress[alice]=50 tokensByAddress[bob] =50 tokensInCirculation = 100 shareHolderRewardsPerTokenScaled = 1 rewardsLastClaimedValue[alice]=1 rewardsLastClaimedValue[bob]=1

Assume Alice executed buy() （Ignore platformFee, shareCreatorFee）

1. fee = 100
2. _getRewardsSinceLastClaim(alice) = 0
3. _splitFees(_tokenCount = tokensInCirculation = 100)
   • shareHolderRewardsPerTokenScaled = 1 + （100/100） = 2
4. rewardsLastClaimedValue[alice]= 2 （update to new shareHolderRewardsPerTokenScaled）

Subsequently, if alice and bob go to get rewards, the results are as follows:

1. _getRewardsSinceLastClaim(alice) = 0
2. _getRewardsSinceLastClaim(bob) = 50 ( 50 * (2-1) = 50 )

As shown above: the fee lost 100-50 =50 The correct one should be _splitFees(_tokenCount = (tokensInCirculation - tokensByAddress[bob] )) = _splitFees(50)

Impact

The allocation of _splitFees does not subtract the current user's share, causing this part of the share to be lost and locked in the contract.

POC

add to Market.t.sol

    function testBuyFeeLoss() public {
        //0.init alice == jack == 0.001
        address jack = address(3);
        testCreateNewShare();
        token.transfer(alice, 0.1e18);
        token.transfer(jack, 0.1e18);

        vm.startPrank(alice);
        token.approve(address(market), 1e18);
        market.buy(1, 1);
        market.claimHolderFee(1);
        vm.stopPrank();

        vm.startPrank(jack);
        token.approve(address(market), 1e18);        
        market.buy(1, 1);
        market.claimHolderFee(1);
        vm.stopPrank();
        //end of init

        //1. jack new buy
        (,uint fee) = market.getBuyPrice(1,1);
        vm.startPrank(alice);
        market.buy(1, 1);
        vm.stopPrank();        
        
        //2.get diff after claim
        uint beforeAlice = token.balanceOf(alice);
        uint beforeJack = token.balanceOf(jack);
        vm.prank(alice);
        market.claimHolderFee(1);
        vm.prank(jack);
        market.claimHolderFee(1);

        //3. check get reward will equal but not
        assert(fee * 3_300 / 10000 == (token.balanceOf(alice) - beforeAlice) + (token.balanceOf(jack) - beforeJack));

    }  

$ forge test --match-test testBuyFeeLoss

Running 1 test for src/test/Market.t.sol:MarketTest
[FAIL. Reason: Assertion violated] testBuyFee() (gas: 583301)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.17ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in src/test/Market.t.sol:MarketTest
[FAIL. Reason: Assertion violated] testBuyFee() (gas: 583301)

Encountered a total of 1 failing tests, 0 tests succeeded

Recommended Mitigation

There are two choices for modification:

1. The current user can also allocate the current fee, which seems quite reasonable.

    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);
+       uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

or 2. The current user cannot get the current fee, _splitFees() needs to subtract the current user's balance.

    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);
+       _splitFees(_id, fee, shareData[_id].tokensInCirculation - tokensByAddress[_id][msg.sender]);

        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability details

in createNewShare() We need to pass in _shareName, and it will be judged that if the same _shareName already exists, creation is not allowed.

    function createNewShare(
        string memory _shareName,
        address _bondingCurve,
        string memory _metadataURI
    ) external onlyShareCreator returns (uint256 id) {
        require(whitelistedBondingCurves[_bondingCurve], "Bonding curve not whitelisted");
@>      require(shareIDs[_shareName] == 0, "Share already exists");
        id = ++shareCount;
        shareIDs[_shareName] = id;
        shareData[id].bondingCurve = _bondingCurve;
        shareData[id].creator = msg.sender;
        shareData[id].metadataURI = _metadataURI;
        emit ShareCreated(id, _shareName, _bondingCurve, msg.sender);
    }

The problem is that the protocol can enter a mode that anyone can be ShareCreator, when shareCreationRestricted=false. In this mode, if there are malicious attackers who always front-run other users and maliciously pass in the same _shareName it will cause the other user to be unable to create any shares.

Impact

In the shareCreationRestricted=false mode, malicious users can DOS attack, causing others to be unable to create share.

Recommended Mitigation

It is recommended to consider charging a certain fee for creating shares or cancel the _shareName restriction

Assessed type

Context