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

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/main/asD/src/asD.sol#L74-L77

Vulnerability details

Description

When calculating the amount of interest gathered this calculation is done: asD::withdrawCarry:

File: asD/src/asD.sol

73:        uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent(); // Scaled by 1 * 10^(18 - 8 + Underlying Token Decimals), i.e. 10^(28) in our case
74:        // 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
75:        uint256 maximumWithdrawable = (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) /
76:            1e28 -
77:            totalSupply();

The issue here is that this is based on the compund deploy on mainnet where the cTokens ave 8 decimals. However if you look at the CLM deployment on Canto:

https://docs.canto.io/evm-development/contract-addresses

cNote:

https://tuber.build/address/0xEe602429Ef7eCe0a13e4FfE8dBC16e101049504C

cNote has 18 decimals and an exchange rate of 1004039816344002600 (1.004e18) at time of writing.

This will cause the call to withdrawCarry to revert on underflow in the calculation (cNoteBalance * exchangeRate) / 1e28 - totalSupply(). As cNoteBalance, exchangeRate, and totalSupply() will have 18 decimals:

e18 * e18 / 1e28 - e18 -> e36 / 1e28 - e18 -> e8 - e18, underflow.

Impact

Calling withdrawCarry will always revert when using cNote on Canto.

Proof of Concept

Test in asD/src/test/asDcNoteTest.t.sol:

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity >=0.8.0;

import {asD} from "../asD.sol";
import {CTokenInterface, CErc20Interface} from "../../interface/clm/CTokenInterfaces.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "forge-std/Test.sol";

contract asDcNoteTest is Test {

    address cNote = makeAddr("cNote");
    address note = makeAddr("note");

    asD asd;

    function setUp() public {
        asd = new asD("asD", "asD", address(this), cNote, address(0));

        // basic mocks
        vm.mockCall(cNote, abi.encodeWithSelector(CErc20Interface.mint.selector), abi.encode(0));
        vm.mockCall(cNote, abi.encodeWithSelector(CErc20Interface.redeemUnderlying.selector), abi.encode(0));
        vm.mockCall(cNote, abi.encodeWithSignature("underlying()"), abi.encode(note));

        vm.mockCall(note, abi.encodeWithSelector(IERC20.transfer.selector), abi.encode());
        vm.mockCall(note, abi.encodeWithSelector(IERC20.approve.selector), abi.encode());
        vm.mockCall(note, abi.encodeWithSelector(IERC20.allowance.selector), abi.encode(0));

        // exchange rate picked from cNote contract at time of writing:
        // https://tuber.build/address/0xEe602429Ef7eCe0a13e4FfE8dBC16e101049504C
        vm.mockCall(cNote, abi.encodeWithSelector(CTokenInterface.exchangeRateCurrent.selector), abi.encode(1004034334164328300));

        // minted 1e18 at an earlier stage, should leave 4034334164328300 (0.004e18) as interest
        vm.mockCall(cNote, abi.encodeWithSelector(CTokenInterface.balanceOf.selector,address(asd)), abi.encode(1e18));
    }

    function testWithdrawRealcNoteCarry() public {
        // mint 1 asD token, this will set total supply to 1e18
        asd.mint(1e18);

        // arithmetic underflow or overflow
        vm.expectRevert();
        asd.withdrawCarry(0);
    }
}

Tools Used

Manual audit, canto documentation and deployed contracts

Recommended Mitigation Steps

Consider changing the scaling factor to be 1e18 (1 * 10^(18 - 18 + Underlying Token Decimals)

Assessed type

Decimal

Lines of code

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

Vulnerability details

Description

When buying a share, first the rewards are calculated for the user, then the fees are split, and lastly the new rewards per token are saved for the user:

Market::buy:

File: 1155tech-contracts/src/Market.sol

154:        // The reward calculation has to use the old rewards value (pre fee-split) to not include the fees of this buy
155:        // The rewardsLastClaimedValue then needs to be updated with the new value such that the user cannot claim fees of this buy
156:        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
157:        // Split the fee among holder, creator and platform
158:        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
159:        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

The order of this is, as the comment says, to ensure that you don't earn the fees for the shares you are buying.

This comes with an unintended side effect though. If you already have shares, these shares do not earn fees for the purchase that is being made either. Rather, the fees these shares would have earned will be stuck in the contract.

Imagine this scenario, there is 1 share in total and the same user buys a new share. The fees for that purchase will be 100% allocated to the one existing share at time of split. Since it uses the old rewardsSinceLastClaim though, the user (who owns both the soon to be bought share and the previous one) will not get this fee.

Since no one else can claim it, the tokens will be stuck in the contract.

Impact

When a user already own one or multiple share, these shares will not get rewards for subsequent purchases the same user makes. Instead those rewards will be stuck forever in the Market contract.

Proof of Concept

PoC that can be pasted in Market.t.sol:

    function testRewardsLeftInContract() public {
        testCreateNewShare();
        token.approve(address(market), 1e18);

        // user buys a share
        market.buy(1, 1);

        uint256 balanceBefore = token.balanceOf(address(this));
        uint256 purchasePrice = 2*LINEAR_INCREASE + 2*LINEAR_INCREASE/10;
        // user buys another share
        // since they own the only share here, their existing share should get all the rewards from this purchase
        market.buy(1, 1);
        // which it didn't
        assertEq(token.balanceOf(address(this)) + purchasePrice - balanceBefore,0);

        // all shares are sold, and all rewards are collected, this should empty the contract of tokens
        market.sell(1, 2);
        market.claimHolderFee(1);

        vm.prank(bob);
        market.claimCreatorFee(1);

        market.claimPlatformFee();

        // however the missed rewards are stuck in the contract
        assertEq(token.balanceOf(address(market)),66e12);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider either allowing a user to earn from fees on their own purchases by doing _splitFees before _getRewardsSinceLastClaim in buy.

Or, remove the current users shares from the _splitFee calculation:

-       _splitFees(_id, fee, shareData[_id].tokensInCirculation);
+       _splitFees(_id, fee, shareData[_id].tokensInCirculation - tokensByAddress[_id][msg.sender]);

Assessed type

Math