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

Lines of code

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

Vulnerability details

Impact

withdrawCarry() calculates underlying amount which corresponds to contract, and subtracts deposited amount from users to get NOTE amount which corresponds to accrued interest. Because admin can withdraw only that accrued interest.

cToken has exchangeRate which is price cToken / UnderlyingToken. Actually exchangeRate has 1e18 precission, however withdrawCarry() assumes it's 1e28.

As a result, admin is allowed to withdraw amount 1e10 times less than intended, which is close to dust.

Proof of Concept

Here it uses 1e28 scale:

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

Here you can call function cNOTE.exchangeRateStored() which currently receives ~1.004e18

Tools Used

Manual Review

Recommended Mitigation Steps

    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
+       uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent(); // Scaled by 1e18
        // 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 -
-           1e18 -
            totalSupply();
        if (_amount == 0) {
            _amount = maximumWithdrawable;
        } else {
            require(_amount <= maximumWithdrawable, "Too many tokens requested");
        }
        ...
    }

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

When user buys or sells shares, mints or burns nft - price that he pays/receives depends on current tokenCount, i.e. number of outstanding tokens. More outstanding tokens - price of buy/mint is higher, less outstanding tokens - price of sell/burn is less.

There is no mechanism for user to specify what price he is willing to pay or receive, as a result he can pay more than expected on buy and mint, because number of outstanding tokens increased before user's transaction execution. And vice versa with sell/burn.

Proof of Concept

Here you can see that price and fee depend on shareData[_id].tokenCount. Greater tokenCount, greater price and fee:

LinearBondingCurve.sol:
    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;
        }
    }
--------------------------------------------------------------------------------------------
Market.sol:
    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);
    }

    function getNFTMintingPrice(uint256 _id, uint256 _amount) public view returns (uint256 fee) {
        address bondingCurve = shareData[_id].bondingCurve;
        (uint256 priceForOne, ) = IBondingCurve(bondingCurve).getPriceAndFee(shareData[_id].tokenCount, 1);
        fee = (priceForOne * _amount * NFT_FEE_BPS) / 10_000;
    }

There is no way for user to specify maxAmount he's willing to pay and minAmount he's willing to receive in user facing functions buy(), sell(), mintNFT(), burnNFT().

Tools Used

Manual Review

Recommended Mitigation Steps

Add argument uint256 minOutput to functions burnNft() and sell(). Add argument uint256 maxInput to functions mintNft() and buy(). And check it.

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

Anybody can create shares when share creation is not restricted. There can be problems because griefer can frontrun user's transaction and become shareCreator for that share with user's shareName and metadataUri. While user's transaction reverts

As a result, attacker can 1) block creation of shares for other users, 2) create shares with user's shareName and metadataUri. In case 2 attacker becomes shareCreator and receives 33% fee instead of user.

Proof of Concept

Share creation requires empty id for current _shareName, i.e. _shareName is unique per id. Attacker can frontrun user, specifying user's _shareName and become shareCreator because of this require:

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

Attacker is able to block any share creation and become the only who creates shares. Additionally he becomes the only shareCreator who receives fee in protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

The simplest way is to completely remove mapping shareIDs. However it may be necessary for backend.

Generally speaking, there shouldn't be any parameter that is user-supplying and unique. Instead mapping shareIDs should be replaced with something different.

Assessed type

MEV