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

Lines of code

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

Vulnerability details

tl;dr

asD::withdrawCarry assumes that the exchange rate returned by the cNote contract will be scaled by 1e28, but in reality it will be only scaled by 1e18. It will cause withdrawCarry to always revert with Integer Underflow, which means that owner won't ever be able to withdraw the accrued interest.

Detailed description

When we analyse the asD::withdrawCarry we will see the following code:

    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");
        }
        // Technically, _amount can still be 0 at this point, which would make the following two calls unnecessary.
        // But we do not handle this case specifically, as the only consequence is that the owner wastes a bit of gas when there is nothing to withdraw
        uint256 returnCode = CErc20Interface(cNote).redeemUnderlying(_amount);
        require(returnCode == 0, "Error when redeeming"); // 0 on success: https://docs.compound.finance/v2/ctokens/#redeem
        IERC20 note = IERC20(CErc20Interface(cNote).underlying());
        SafeERC20.safeTransfer(note, msg.sender, _amount);
        emit CarryWithdrawal(_amount);
    }

The comment in the first line says that the result returned by the exchangeRateCurrent will be "Scaled by 1 * 10^(18 - 8 + Underlying Token Decimals), i.e. 10^(28) in our case". This statement has its source in the Compound docs, where it's written that: RETURN: The current exchange rate as an unsigned integer, scaled by 1 * 10^(18 - 8 + Underlying Token Decimals).

It is indeed true for CErc20 and CEther contracts deployed on Ethereum (try for example exchangeRateStored on cETH), because they indeed have 8 decimals (see decimals on the cETH).

The problem is that the CNote contract has 18 decimals, which means that the result returned by exchangeRateCurrent will be scaled by 10^(18 - 18 + 18) = 1e18, not 1e28. Hence the formula given in the Compound docs doesn't apply here and we have to substitute 8 with 18 in it.

Impact

withdrawCarry will always revert when calculating maximumWithdrawable, hence owner will never be able to receive the income. It's because (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) / 1e28 will be less than totalSupply() in almost all cases, unless the contract has ~$10 000 000 000 per each token from the totalSupply() which is irrational.

I'm submitting this issue as High because the entire point of the asD contract is to mint cNotes out of received Note tokens in order to get profits and the only way to claim the profits is by the withdrawCarry function.

Proof of Concept

The address of the CNote contract is 0xEe602429Ef7eCe0a13e4FfE8dBC16e101049504C, which can be confirmed in the Canto docs.

We can either click exchangeRateStored here to quickly check that the result is indeed scaled by 1e18.

More detailed proof is shown below.

When we look at the CNote implementattion contract, we will see that when exchangeRateCurrent is called, the following code executes:

    function exchangeRateCurrent() override public nonReentrant returns (uint) {
        accrueInterest();
        return exchangeRateStored();
    }

So, the value returned by exchangeRateStored is returned. The code of exchangeRateStored is shown below:

    function exchangeRateStored() override public view returns (uint) {
        return exchangeRateStoredInternal();
    }

So, another function is called - exchangeRateStoredInternal, which code looks as follows:

    /**
     * @notice Calculates the exchange rate from Note to cNote
     * @dev This function does not accrue efore calculating the exchange rate
     * @return calculated exchange rate scaled by 1e18
     */
    function exchangeRateStoredInternal() virtual internal view returns (uint) {
        uint _totalSupply = totalSupply;
        if (_totalSupply == 0) {
            /*
             * If there are no tokens minted:
             *  exchangeRate = initialExchangeRate
             */
            return initialExchangeRateMantissa;
        } else {
            /*
             * Otherwise:
             *  exchangeRate = (totalCash + totalBorrows - totalReserves) / totalSupply
             */

            uint totalCash = getCashPrior();

            uint cashPlusBorrowsMinusReserves = totalCash + totalBorrows - totalReserves;
            uint exchangeRate = cashPlusBorrowsMinusReserves * expScale / _totalSupply;

            return exchangeRate;
        }
    }

As can be seen from the comment above the function, the value returned is scaled by 1e18, not 1e28.

Tools Used

VS Code, Canto block explorer

Recommended Mitigation Steps

Change 1e28 in asD::withdrawCarry to 1e18.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

tl;dr

Anybody, including arbitrage bots, can frontrun every single call to Market::buy and immediately after a user buys some tokens, they can call sell the same amount of tokens and get profit in the expense of the user.

Detailed description

Users can buy tokens on the market using the Market::buy function:

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

        [...]
    }

As we see, it calls getBuyPrice, which contains the following code:

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

As can be noted, it returns the values returned by bondingCurve.getPriceAndFee(shareData[_id].tokenCount + 1, _amount). getPriceAndFee looks like this:

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

As can deduced from the code above, token price is dependent on the current supply of tokens and is roughly (not counting fees) equal to priceIncrease * (shareCount + (shareCount + 1) + ... + (shareCount + amount - 1)). So, the more tokens are in total supply, the higher is the price of new tokens.

The sell function that enables users to sell tokens works exactly in the same way - if we ignore fees, then buying k tokens and selling them immediately would not impact user's balance.

The problem is that calls to buy may be frontrunned. In order to see this, consider the following scenario (for simplicity, we ignore fees):

1. Alice is the first (not necessary, but will make the computations simpler) user of the protocol and she wants to buy 10 tokens.
2. Attacker sees this and frontruns Alice and buys 10 tokens for himself. He pays priceIncrease * (1 + 2 + 3 + ... + 10) = 55 * priceIncrease.
3. Alice's transaction is executes and she pays priceIncrease * (11 + 12 + ... + 20) = 155 * priceIncrease.
4. Attacker now sells his tokens. The selling price will be exactly the same as Alice paid for buying her tokens, so it will equal 155 * priceIncrease. Attacker earned 100 * priceIncrease and if Alice decides to sell her tokens, she will sell each token for a significantly cheaper price than she paid when she bought it.

It's also worth to notice that arbitrage bots can even perform sandwitch attacks on themselves. To see this consider this example (we avoid fees once again and we assume that priceIncrease = 1 for simplicity):

1. Alice wants to buy 10 tokens.
2. attacker1 wants to frontrun her and creates a batch of transactions where he first buys, then Alice buys and the he sells tokens.
3. attacker2 sees this and creates another batch of transactions where he first buys tokens, then the batch created by attacker1 executes and then he sells.
4. So, the first transaction that will execute is buy of 10 tokens performed by attacker2 - he pays 55.
5. Then, attacker1 buys 10 tokens - he pays 155.
6. Then, Alice buys 10 tokens - she pays 255.
7. Then, attacker1 sells 10 tokens for 255 - he profits 100.
8. Then, attacker2 sells 10 tokens for 155 - he profits 100.
9. Alice first bought tokens for 255 and she can now sell for only 55.

We can consider even more attackers in our scenario and each additional one will make Alice to lose even more money.

Note: It is also possible that users will buy for higher prices by an accident, if someone unintentionally frontruns them with buy or with burning NFT increasing token total supply. I'm not reporting it as a separate issue since it's less severe than the issue currently reported and the root cause is the same.

Impact

The impact is that each user may be frontrunned - it means that there will be a lot of arbitrage bots that will rob users from their money. Moreover, the amount lost for users can be arbitrarily high - since arbitrage bots can frontrun themselves, they may inflate prices to astronomical levels, so even if user only buys 1 token, he may lose a lot of money - the only thing that limits the attackers from stealing unbounded amount of money is block gas limit.

Each user who buys some tokens will not be able to sell them for a good price (at least until some new users enter the protocol and increase the token price) - it means that each user will lose money right away at the beginning, just after buy is executed.

Since the attack is trivial to execute and the fact that users are lost even after the first interaction with the protocol (which will make them reluctant to use it at all) and they can lose a lot of money, I'm submitting the issue as High.

Proof of Concept

Please add the following function to the MockERC20 in Market.t.sol:

    function mint(address to, uint amount) public
    {
        _mint(to, amount);
    }

Please add and run the following test:

    function testSandwitch() public
    {
        // create the attacker account
        address attacker = address(0x1234);

        // mint some tokens so that users can use them
        token.mint(alice, 1e19);
        token.mint(attacker, 1e19);

        // approve tokens to the market
        vm.prank(alice);
        token.approve(address(market), type(uint).max);

        vm.prank(attacker);
        token.approve(address(market), type(uint).max);

        // create share so that users can but tokens on the market
        testCreateNewShare();

        // now, alice plans to buy 10 tokens, but attacker frontruns her and buys 10 tokens before her
        vm.prank(attacker);
        market.buy(1, 10);

        vm.prank(alice);
        market.buy(1, 10);

        // just after alice buys, attacker sells his tokens and receives a profit
        vm.prank(attacker);
        market.sell(1, 10);

        assert(token.balanceOf(attacker) > 1e19);
        assert(token.balanceOf(alice) < 1e19);
        console.log("token.balanceOf(attacker)", token.balanceOf(attacker));
        console.log("token.balanceOf(alice)", token.balanceOf(alice));
    }

Tools Used

VS Code

Recommended Mitigation Steps

Add the maxAmount parameter to the buy function so that users can specify the maximum amount of money they are willing to pay for tokens. While sandwitch attacks will still be possible unless users specify very strict amounts, their impact will be significantly reduced.

Assessed type

Other

Lines of code

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

Vulnerability details

tl;dr

Since tokens will be more expensive to buy when the total supply increases, some malicious users can buy a lot of tokens right after protocol deployment just in order to dump them later on for a higher price when new users arrive. These new users will then be unable to sell their tokens for an acceptable price.

Detailed description

The current mechanism of calculating token prices in Market::buy and Market::sell makes token price to increase a lot when token supply increases - for example minting 1000th token will be roughly 1000 times more expensive than buying the first one. it can be verified by analysing the LinearBondingCurve::getPriceAndFee:

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

It encourages users to buy a lot of tokens right after protocol deployment. This way, when new users enter, they (early users who bought a lot of tokens at the beginning) can dump their tokens and receive profit. But because of their actions, new users will pay significantly more for tokens than they would normally do.

In order to see this, consider the scenario:

1. Protocol is deployed and configured.
2. Attacker is the first user and buys 1000 tokens.
3. Subsequent protocol users will have to pay significantly more than they normally would - if we ignore fees, it would be 1001 * priceIncrease for the first NFT instead of 1 * priceIncrease, which is over 1000 more.
4. After each such buy, attacker can dump a token, earning 1000 * priceIncrease each time.
5. It means that users will not only have to pay more money for tokens, but if they want to sell them, they will have to do this for less than they paid for them (at least until new users arrive and buy more tokens).

Impact

Current fee calculation mechanism encourages unhealthy behaviour among users - users are incentivised to speculate and buy tokens early on only in order to sell them later, without really using the protocol (the additional incentive is that users with high token balance will get significant amount of protocol fees). This behaviour also harms innocent users discouraging them from using the protocol at all. In the scenario that I have described in the previous section, no "normal" user will be able to buy tokens for prices lower than 1000 * priceIncrease - only attacker will do so.

Note that attacker doesn't risk a lot here, as he will be able to sell his tokens at any time, in the worst case, for the price he initially bought them - he will only pay the fees, but they aren't high enough in order to discourage such attacks, especially that he will get a lot of fees paid by other users since he has a lot of tokens.

Note: This issue is different than the one about possible sandwitch attack that I submitted earlier. First of all, because the fix for that issue doesn't fix the problem described here, and secondly, this issue is about the design flaw that encourages unhealthy behaviours among users, that makes other users to be harmed.

Proof of Concept

First, please add the following function to the MockERC20 in Market.t.sol:

    function mint(address to, uint amount) public
    {
        _mint(to, amount);
    }

Then, please put the following test to Market.t.sol and run it:

    function testUnhealthy() public
    {
        uint INITIAL_BALANCE = 1e22;

        // create the attacker account
        address attacker = address(0x1234);

        // mint some tokens so that users can use them
        token.mint(alice, INITIAL_BALANCE);
        token.mint(attacker, INITIAL_BALANCE);

        // approve tokens to the market
        vm.prank(alice);
        token.approve(address(market), type(uint).max);

        vm.prank(attacker);
        token.approve(address(market), type(uint).max);

        // create share so that users can but tokens on the market
        testCreateNewShare();

        // attacker buys first 1000 tokens
        vm.prank(attacker);
        market.buy(1, 1000);

        // alice has to buy for a higher price then she normally would
        vm.prank(alice);
        market.buy(1, 10);
        // attacker can either wait for more users or just sell some of his tokens in order to receive some profit
        // assume that he waits longer
        
        // we now simulate another users entering the protocol - in order to make it simple, we will do it by
        // buying another 990 tokens by alice - normally many users would do it over longer time interval
        vm.prank(alice);
        market.buy(1, 990);
        
        // attacker sell all of his tokens (he can do it gradually over time, but in order to keep things
        // simple, we will simulate this by selling all of his tokens at once)
        vm.prank(attacker);
        market.sell(1, 1000);

        assert(token.balanceOf(attacker) > INITIAL_BALANCE);
        console.log("token.balanceOf(attacker)", token.balanceOf(attacker));
    }

Tools Used

VS Code

Recommended Mitigation Steps

It's not so easy to fix this problem as incentives for the early users should still exist.

The best fix that comes to my mind is to limit the amount of tokens that users can buy for some time after each share has been created (say 1 week) to 1 and after that time allow anybody to buy as many tokens as they want. It will still be possible to buy tokens from many accounts at the beginning, but it will be more costly:

• if we assume that the attacker intends to buy 1000 tokens, he would have to create 1000 accounts, send some tokens for each of these accounts and then make 1000 buy transactions - doing so many transactions will be costly and annoying, so it will deter the attackers. Moreover, if the attacker even managed to get 1000 tokens (one per each account), then he will not benefit from protocol fees (as gas fees for 1000 calls to claimHolderFee will consume the entire fees earned).

Assessed type

Other