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

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The Market.sell() function lacks slippage control. Note that the price of a share depends on shareData[_id].tokenCount. Therefore, it is important to have a slippage control to make sure that the seller will get a minimum amount of tokens back as a result of selling of the shares.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

A user uses the sell() function to sell tokens.

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

The price of selling shares depend on shareData[_id].tokenCount on a linear curve and calculated by the following getSellPrice() function:

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

However, the sell() function lacks a slippage control for the price, as a result, the user might sell the shares and get very little payment tokens back due to lower price. In particular, another user might front-run the selling transaction and sell first with a better price. As a result, the user will lose funds.

Example:

1. The current shareData[_id].tokenCount = 1000, and user Frank will sell his 50 shares;

2. User Alice fronts run Frank's transaction and sell 100 shares first, as a result, shareData[_id].tokenCount = 900.

3. Now when Frank sells his shares, he sells at a much lower price than he expected, since there is no slippage control, Frank's transaction will go through successfully and Frank receives much less payment tokens then he expected. Loss of funds

Tools Used

VSCode

Recommended Mitigation Steps

Add a slippage control to the sell() function so that when the price is lower than expected (for example, due to front-running), the sell() will revert and and no selling will occur.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The buy() function lacks a slippage control, and when front-run, a user might pay more than he expected.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The buy() function allows a user to buy shares using the payment tokens.

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

However, the price of the shares follow the linear bounding curve, later shares will be more expensive than early shares. The price of shares are determined by the following getBuyPrice() function:

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

Unfortunately, the buy() function lacks the slippage control, as a result, another user might front-run the transaction and as a result, the first user will pay more than he expected to buy the shares.

For example:

1. Suppose shareData[_id].tokenCount = 100; and Frank likes to buy 100 shares;

2. Another user Alice front-runs Frank's transaction and purchased 900 shares, as a result, shareData[_id].tokenCount = 1000.

3. Now when Frank purchased his 100 shares, he needs to pay much higher price than he started since now shareData[_id].tokenCount = 1000. This occurs since there is no slippage control for the buy(), assume that Frank has given a high approval amount, which occurs a lot in practice to improve user experience. As a result, the transaction will go through and Frank will pay more than he expected. Loss of funds.

Ideally, Frank's should be able to revert the transaction if he needs to pay more than he expected.

Tools Used

VSCode

Recommended Mitigation Steps

Add a slippage control to buy() so that the function will revert when the amount of payment is greater than the threshhold.

Assessed type

Invalid Validation

QA1. We need to check whether rewardsSinceLastClaim + price - fee == 0 for the following line in function sell(). The function should fail when rewardsSinceLastClaim + price - fee == 0 to avoid not worthy sell.

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

Mitigation:

function sell(uint256 _id, uint256 _amount) external {
        (uint256 price, uint256 fee) = getSellPrice(_id, _amount);
        // Split the fee among holder, creator and platform
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        // The user also gets the rewards of his own sale (which is not the case for buys)
        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

        shareData[_id].tokenCount -= _amount;
        shareData[_id].tokensInCirculation -= _amount;
        tokensByAddress[_id][msg.sender] -= _amount; // Would underflow if user did not have enough tokens

        // Send the funds to the user
+       uint amt = rewardsSinceLastClaim + price - fee;

-        SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim + price - fee);

+        if (amt > 0) SafeERC20.safeTransfer(token, msg.sender, amt);
+        else revert("no zero sell.");

        emit SharesSold(_id, msg.sender, _amount, price, fee);
    }

QA2. asDFactory.create() does not prevent that two asD contracts might have the same name and symbol.

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

QA3. getPriceAndFee() have the issue of division precision loss issue.

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/bonding_curve/LinearBondingCurve.sol#L14-L25

The problem is in the following line:

fee += (getFee(i) * tokenPrice) / 1e18;

The division of 1e18 should be performed after the loop is completed to avoid early division rounding error.

Mitigation: the division of 1e19 is performed outside:

 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;
+            fee += (getFee(i) * tokenPrice);
        }
+            fee = fee / 1e18;
    }

QA4: It is better to check amount > 0 to avoid false event emit:

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

QA6: The event emitting should be inside the if statement to avoid false event emitting

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

QA7. We need to emit an event for this function:

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

QA8. Both mintNFT() and burnNFT() lacks slippage control for the fee. Note that the fee is not a fixed value, its value depends on shareData[_id].tokenCount.

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

Therefore, if the function is front-run by another buy() and mintNFT() functions, then shareData[_id].tokenCount will increase, and as a result, the fee will increase as well. A user might pay more fee than he expected due to such front-runs.

Mitigation: Add a slippage control so that the user can control the maximum amount of fee he is willing to pay.

QA9: The changeBondingCurveAllowed() will not be able to disable previously boundcurves that have been used in some share creation. It can only used to prevent future share creation to use the curve again.

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

QA10: changeShareCreatorWhitelist() cannot disable existing creators for their already created shares. It can only prevent the creator to create more shares in the future.

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

QA11: All the four functions buy(), sell(), mintNFT() and burnNFT() calls _splitFees() to split the fees, but when shareData[_id].tokensInCirculation ==0, shareHolderFee will be added to plantformFee.

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

In this case, a user can front-run these transactions with a buy() to buy a share so that shareData[_id].tokensInCirculation > 0 and therefore effectively be able to steal all such shareHolderFee. in these situations.

Mitigation: implement a snapshot mechanism for market so that a user can not just suddently buy shares and enjoy rewards immediately. See ERC20Snapshot for an example:

[https://docs.openzeppelin.com/contracts/3.x/api/token/erc20#ERC20Snapshot(https://docs.openzeppelin.com/contracts/3.x/api/token/erc20#ERC20Snapshot)

G1. getPriceAndFeeChange(): perform the division of 1e18 outside of loop can save gas:

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/bonding_curve/LinearBondingCurve.sol#L14-L25

mitigation:

 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;
+            fee += (getFee(i) * tokenPrice);
        }
+            fee = fee / 1e18;

    }

G2. getPriceAndFeeChange(): Actually the price can be calculated more straigforwardly outside of the loop since we just need to sum of a sequence of numbers that increase by priceIncrease.

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/bonding_curve/LinearBondingCurve.sol#L14-L25

 function getPriceAndFee(uint256 shareCount, uint256 amount)
        external
        view
        override
        returns (uint256 price, uint256 fee)
    {
  
        // sum up a sequence of numbers that increase by delta
+       price = (priceIncrease * shareCount + priceIncease * (shareCount + amount -1)) * amount / 2; 

        for (uint256 i = shareCount; i < shareCount + amount; i++) {
            uint256 tokenPrice = priceIncrease * i;
            fee += getFee(i) * tokenPrice;
        }
         fee = fee / 1e18;

    }

G3. LinearBondingCurve: The getFee() function should be changed to getDivisor() as follows:

  function getDivisor(uint256 shareCount) public pure override returns (uint256 divisor) {
        if (shareCount > 3) {
            divisor = log2(shareCount);
        } else {
            divisor = 1;
        }
 }

Then, getPriceAndFee() can be simplifed as follows, saving gas and more readable.

 function getPriceAndFee(uint256 shareCount, uint256 amount)
        external
        view
        override
        returns (uint256 price, uint256 fee)
    {

        price = (priceIncrease * shareCount + priceIncease * (shareCount + amount -1)) * amount / 2; 

        uint256 tokenPrice;
        for (uint256 i = shareCount; i < shareCount + amount; i++) {
            tokenPrice = priceIncrease * i;
            fee += tokenPrice / getDivisor(i);
        }
         
        fee = fee / 10;
    }

G4. The above getPriceAndFee() can further be simplified as follows to save gas based on the idea that: only the first divisor needs to be obtained, and future divisors can be calculated without calling getDivisor(), just add divisor by 1 when i is doubled.

 function getPriceAndFee(uint256 shareCount, uint256 amount)
        external
        view
        override
        returns (uint256 price, uint256 fee)
    {

        price = (priceIncrease * shareCount + priceIncease * (shareCount + amount -1)) * amount / 2; 
        // the right side can be simplified too if necessary
        
        uint256 tokenPrice = priceIncrease * shareCount;
        uint256 divisor = getDivisor(shareCount);
        uint256 j = shareCount < 1; // the next time that divisor needs to increase by 1
        fee += tokenPrice / divisor;
  
        for (uint256 i = shareCount + 1; i < shareCount + amount; i++) {
            tokenPrice += priceIncrease; // instead of multipication;
            if(i == j) {divisor++; j = j < 1;} // time to increase divisor by 1 and double j 
            fee += tokenPrice / divisor;
        }
         
        fee = fee / 10;
    }

G5. We should allow the creator to buy shares too since it is an easy get-around anyway. Better to delete the following line to save gas:

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