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

Lines of code

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

Vulnerability details

Impact

asD contract allows users to deposit NOTE tokens in exchange for asD tokens, what is happening under the hood is that we mint cNOTE tokens (compound like lending) and store them in the contract while users receive asD tokens with 1:1 ratio to NOTE.

    function mint(uint256 _amount) external {
        CErc20Interface cNoteToken = CErc20Interface(cNote);
        IERC20 note = IERC20(cNoteToken.underlying());
        SafeERC20.safeTransferFrom(note, msg.sender, address(this), _amount);
        SafeERC20.safeApprove(note, cNote, _amount);
        uint256 returnCode = cNoteToken.mint(_amount);
        // Mint returns 0 on success: https://docs.compound.finance/v2/ctokens/#mint
        require(returnCode == 0, "Error when minting");
        _mint(msg.sender, _amount);
    }

cNOTE generates interest for it's holders, which the asD contract owner can withdraw, but he must maintain a 1:1 ratio and not withdraw too much. Unfortunately the formula that we use for withdrawable amount is incorrect https://github.com/code-423n4/2023-11-canto/blob/main/asD/src/asD.sol#L73-L77

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

we assume that exchange rate has 10^28 decimals while in reality it's 10^18, this will result in inability to withdraw accrued interest because this part (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) / 1e28 will be greatly reduced.

Proof of Concept

Here we can see https://tuber.build/address/0x4e71A2E537B7f9D9413D3991D37958c0b5e1e503?tab=read_contract https://tuber.build/address/0xEe602429Ef7eCe0a13e4FfE8dBC16e101049504C?tab=read_proxy that both NOTE and cNOTE have 18 decimals, this will give us 10^(18 - 18 + 18) decimals for the exchange rate

Tools Used

Manual review

Recommended Mitigation Steps

use 10^18 divisor instead of 10^28 in https://github.com/code-423n4/2023-11-canto/blob/main/asD/src/asD.sol#L76

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/Market.sol#L152 https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/Market.sol#L175 https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/bonding_curve/LinearBondingCurve.sol#L20-L21

Vulnerability details

Impact

Market.sol allows share creators to customize a bonding curve for their NFTs, currently there is only one available - LinearBondingCurve.sol. In this curve price is linearly dependent to the current share amount, this means each time someone buys a share it will increase, and decrease whenever someone sells. https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/bonding_curve/LinearBondingCurve.sol#L20-L21

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

This leaves users in a vulnerable state because price at the moment of the transaction may not be equal to the price at the moment of execution.

Proof of Concept

Consider this example: NFT with priceIncrease = 0.1 ETH and tokenCount = 0, let's omit fees for simplicity. Alice decides to buy one share, that will be 0.1 * 1 = 0.1 ETH, unfortunately she was frontrunned by another user who purchased one share for 0.1 ETH, now she must pay 0.1 * 2 = 0.2 ETH, later frontrunner can sell this share for 0.2 ETH and make a profit. Check this forge test

    function testFrontrun() public {
        // prep
        testCreateNewShare();
        token.approve(address(market), 1e18);
        token.mint(alice, 1e18);
        (uint256 price, ) = market.getBuyPrice(1, 1);
        console.log("PRICE BEFORE: ", price);
        // Alice tries to buy one share, but attacker frontruns her
        uint256 attackerBalanceBefore = token.balanceOf(address(this));
        market.buy(1, 1);
        // price doubled
        (price, ) = market.getBuyPrice(1, 1);
        console.log("PRICE AFTER FRONTRUN: ", price);
        vm.startPrank(alice);
        token.approve(address(market), 1e18);
        // Alice spends more tokens than expected
        market.buy(1, 1);
        vm.stopPrank();
        // attacker then sells the share with profit
        market.sell(1, 1);
        uint256 attackerBalanceAfter = token.balanceOf(address(this));
        console.log("PROFIT: ", attackerBalanceAfter - attackerBalanceBefore);
    }

Tools Used

Foundry, I modified MockERC20 token to allow minting to any user

Recommended Mitigation Steps

Allow users to specify maxPrice on buy transactions and minPrice on sell.

    function buy(uint256 _id, uint256 _amount, uint256 _maxPrice) external {
        require(shareData[_id].creator != msg.sender, "Creator cannot buy");
        (uint256 price, uint256 fee) = getBuyPrice(_id, _amount); // Reverts for non-existing ID
        require(price <= maxPrice, "Price too big");
        ...

    function sell(uint256 _id, uint256 _amount, uint256 _minPrice) external {
        (uint256 price, uint256 fee) = getSellPrice(_id, _amount);
        require(price >= minPrice, "Price too small");
        ...

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

Users pay fees for all Market.sol operations. During a sell, mint or burn we first split the fee between all takers (shareholders, share creator and the market owner), and then call _getRewardsSinceLastClaim(_id). This function accrues fee share for the msg.sender

    function _getRewardsSinceLastClaim(uint256 _id) internal view returns (uint256 amount) {
        uint256 lastClaimedValue = rewardsLastClaimedValue[_id][msg.sender];
        amount =
            ((shareData[_id].shareHolderRewardsPerTokenScaled - lastClaimedValue) * tokensByAddress[_id][msg.sender]) /
            1e18;
    }

therefore msg.sender receives past rewards and part of the fee that he paid for the operation. This is not the case for the buy operation. Here we split the fee after calling _getRewardsSinceLastClaim(_id) https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/Market.sol#L158-L159 What if msg.sender already has some tokens? Let's check _splitFees

    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;
        }
        platformPool += platformFee;
    }

here if there are some tokens in circulation (_tokenCount > 0) we update shareHolderRewardsPerTokenScaled, let's assume buyer has 1 token and it's the only one in circulation this will give us shareHolderRewardsPerTokenScaled = shareHolderFee * 1e18, unfortunately no one can claim this reward because later in buy we set https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/Market.sol#L159 rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;. As a result, this reward share will stay at the contract as a dead share.

Proof of Concept

Check this forge test, user buys one share, and then buys 5 more. Fee for buying 5 shares will stay at the contract after everyone claimed their rewards.

    function testBuy() public {
        testCreateNewShare();
        token.approve(address(market), 1e18);
        market.buy(1, 1);
        (, uint256 fee) = market.getBuyPrice(1, 5);
        console.log("FEE: ", fee);
        market.buy(1, 5);
        console.log("MARKET BAL BEFORE CLAIM: ", token.balanceOf(address(market)));
        market.sell(1, 6);
        vm.prank(bob);
        market.claimCreatorFee(1);
        market.claimPlatformFee();
        market.claimHolderFee(1);
        console.log("MARKET BAL AFTER CLAIM AND SELL: ", token.balanceOf(address(market)));
        uint256 shareholderReward = fee * 3300 / 10000;
        assertEq(token.balanceOf(address(market)), shareholderReward);
    }

Tools Used

Foundry, test/Market.t.sol

Recommended Mitigation Steps

Consider calling _getRewardsSinceLastClaim(_id) after _splitFees in buy function, as we already do in other operations.

Assessed type

Token-Transfer