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

Lines of code

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

Vulnerability details

Proof of Concept

Take a look at Market.sol#L171-L189

    function sell(uint256 _id, uint256 _amount) external {
        (uint256 price, uint256 fee) = getSellPrice(_id, _amount);
        // Split the fee among holder, creator and platform
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        // The user also gets the rewards of his own sale (which is not the case for buys)
        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

        shareData[_id].tokenCount -= _amount;
        shareData[_id].tokensInCirculation -= _amount;
        tokensByAddress[_id][msg.sender] -= _amount; // Would underflow if user did not have enough tokens

        // Send the funds to the user
        SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim + price - fee);
        emit SharesSold(_id, msg.sender, _amount, price, fee);
    }

As seen, this function is used whenever a user wants to sell share amounts of a given share ID, issue is that this function does not implement any slippage protection, this allows attackers to use sandwich attacks to manipulate prices and profit at the honest user's expense.

Impact

This could lead to users suffering heavy financial losses, where an attacker manipulates the price before the users tx go through, there is also a potential of this to be two sided, i.e when the attacker front runs and then back run the tx.

Tool used

Manual Review

Recommended Mitigation Steps

A popular fix to sandwiching attacks is to implement a minimum withdrawal amount, in this case that would be to allow the user to specify the smallest value of price they'd accept and revert the tx if their condition is not met.

Assessed type

Token-Transfer

QA Report

Table of Contents

QA-01 The return value of the turnstile.register() should be checked

Impact

In the constructor for some contracts in scope, e.g aSD.sol, the code attempts to register the Turnstile contract if the chain ID is the Canto mainnet or testnet, i.e 7700 / 7701. However, it doesn't check the return value of the turnstile.register()function call. Failing to check the return value can lead to unexpected behaviour if the registration fails or if the function reverts under certain conditions.

Proof of Concept

Using aSD.sol as a case syidy Take a look at

    constructor(
        string memory _name,
        string memory _symbol,
        address _owner,
        address _cNote,
        address _csrRecipient
    ) ERC20(_name, _symbol) {
        _transferOwnership(_owner);
        cNote = _cNote;
        if (block.chainid == 7700 || block.chainid == 7701) {
            // Register CSR on Canto main- and testnet
            Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);
            //@audit
            turnstile.register(_csrRecipient);
        }
    }

Use this search command: https://github.com/search?q=repo%3Acode-423n4%2F2023-11-canto%20turnstile.register(&type=code in order to find all instances of this in scope

Recommended Mitigation Steps

Create a custom error and revert with this if the attempt to register does not go through.

Additionally, consider storing the Turnstile contract address in a configuration file that can be updated externally.

QA-02 asD::withdrawCarry() should direct to Compound's redeeemUnderlying() instead

Impact

Info, incorrect integration of external codebase

Proof of Concept

Take a look at withdrawCarry()

    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);
        //@audit
        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);
    }
}

As seen the function directs to the Compound docs for #redeem where as it should direct it to #redeemUnderlying just like the burn() function

Recommended Mitigation Steps

Correctly integrate external codebase and also make this changes to the withdrawCarry() function

    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
+        require(returnCode == 0, "Error when redeeming"); // 0 on success: https://docs.compound.finance/v2/ctokens/#redeemUnderlying
        IERC20 note = IERC20(CErc20Interface(cNote).underlying());
        SafeERC20.safeTransfer(note, msg.sender, _amount);
        emit CarryWithdrawal(_amount);
    }
}

QA-03 A limit should be attacehd to shareCount

Impact

Low, info on better code structure

Proof of Concept

Take a look at 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;
        }
    }

As seen this function queries and returns the price and fee, one thing to note is that the function does not limit the amount of shareCount it could work with which could lead to it getting error'd due to an OOG.

Recommended Mitigation Steps

Introduce a max share count to protocol

QA-04 getNFTMintingPrice() should align more with it's documentation

Impact

Low/info... confusing codes.

Proof of Concept

Take a look at getNFTMintingPrice()

    /// @notice Returns the price and fee for minting a given number of NFTs. @audit this should return both the price and fee as suggested by the comments, but it instead returns only the fee
    /// @param _id The ID of the share
    /// @param _amount The number of NFTs to mint.
    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;
    }

As seen, the function's name and its comments project this to return both the price to mint an NFT and it's fee, but the function only returns the fee even in all the instances where it's been queried

Recommended Mitigation Steps

Either make the function more aligned to the documentation or better still correct the documentation if this is intended.

QA-05 Assembly code should always be accompanied with comments due to their complex nature

Impact

Info

Proof of Concept

Take a look at LinearBondingCurve::log2()

    function log2(uint256 x) internal pure returns (uint256 r) {
        /// @solidity memory-safe-assembly
        assembly {
            r := shl(7, lt(0xffffffffffffffffffffffffffffffff, x))
            r := or(r, shl(6, lt(0xffffffffffffffff, shr(r, x))))
            r := or(r, shl(5, lt(0xffffffff, shr(r, x))))
            r := or(r, shl(4, lt(0xffff, shr(r, x))))
            r := or(r, shl(3, lt(0xff, shr(r, x))))
            // forgefmt: disable-next-item
            r := or(
                r,
                byte(
                    and(0x1f, shr(shr(r, x), 0x8421084210842108cc6318c6db6d54be)),
                    0x0706060506020504060203020504030106050205030304010505030400000000
                )
            )
        }
    }
}

As seen, this function is used to get the log2 of any value x, issue is that this code block uses assembly code, which is a way lower level than normal solidity code and is highly error prone

Recommended Mitigation Steps

As popularly recommended in the web 3 space, whenever assembly code is used, it should always be accompanied with comments, this should also be implemented with Canto too.

QA-06 Introduce emergency functionalities

Impact

Info

Proof of Concept

Using this search command: https://github.com/search?q=repo%3Acode-423n4%2F2023-11-canto%20pause%20unpause&type=code we can see that all in-scope contracts lack any sort of emergency functionality, whereas adding this could lead to additional centralization, it's advisable cause in the case of a black swan event protocol can easily protect itself from too much harm

Recommended Mitigation Steps

Add emergency functionalities to protocol.

QA-07 Fee calculation should be better documented

Impact

Info, non-understandable piece of code

Proof of Concept

In Market.sol it can be seen that the process for fee calculation is not transparently documented in the contract. Clear comments or documentation regarding how fees are calculated and distributed would be beneficial for both users/developers and auditors.

Recommended Mitigation Steps

Improve fee calculation documentation.

Analysis

Table of Contents

• Approach
• Architecture Overview
• Testability
• Centralization Risks
• Systemic Risks
• Summary of Reported Findings
• Learnt About
• Other Recommendations
• Security Researcher Logistics
• Conclusion

Approach

The first few hours after starting the security review was dedicated to examining the smart contracts in scope, after this quick review, a few hours was spent on researching some of the external integrations, this was then followed by a meticulous, line-by-line security review of the sLOC in scope, beginning from the contracts small in size like asD.sol ascending to the bigger contracts like the Market.sol. The concluding phase of this process entailed interactive sessions with the developers on Discord. This fostered an understanding of unique implementations and facilitated discussions about potential vulnerabilities and observations that I deemed significant, requiring validation from the team.

Architecture Overview

1155tech

• Purpose: 1155tech facilitates the creation of unique shares accompanied by an arbitrary bonding curve. Currently, it supports a linear bonding curve, where the price escalates linearly based on the total share supply. As understood, future updates may introduce additional bonding curve models. Each transaction involves a fee, distributed among the share's creator, the platform, and the current shareholders. Shareholders have the capability to mint an ERC1155 token, incurring a fee proportionate to the token's current market value. They also have the option to burn these tokens later, which also attracts a fee.
• SLOC: 191

Share Creation Process

The Market.createNewShare function is for generating new shares. The process of share creation is flexible, allowing either unrestricted access or limiting it to pre-approved (whitelisted) addresses. Importantly, no fee is levied for establishing new shares.

Token Acquisition

To purchase tokens, users employ the buy function, specifying the desired share ID. This transaction automatically triggers the claiming of any accumulated rewards for the token holder, applicable both during purchase and sale.

Token Disposal

Tokens are sold using the sell function. The applicable fees are subtracted from the selling price. It's important to note that theoretically, fees could exceed the sale price, potentially rendering shares unsellable. This scenario is improbable with a linear bonding curve but could occur with other, more complex curves. In such cases, reverting the transaction is considered acceptable, as there would be no incentive for users to sell under these conditions.

Fee Claiming Mechanisms

Three distinct functions – claimPlatformFee, claimHolderFee, and claimCreatorFee – are available for different stakeholders (platform administrators, shareholders, and share creators, respectively) to claim their respective portions of the accrued transaction fees.

Bonding Curve

• Purpose: The LinearBondingCurve contract, uses the IBondingCurve interface, it implements a bonding curve mechanism where the price per share increases based on a pre-set rate (priceIncrease), calculating the cumulative price and fee for a specified amount of shares, with a fee structure that doesn't increase as fast as the share count, i.e logarithmically relative to the share count, and includes a custom implementation of the log2() function, sourced from Solady's FixedPointMathLib.sol, to support its fee calculation logic.
• SLOC: 45

Application-Specific Dollar (asD)

• Purpose: asD is intended to always be pegged 1:1 with NOTE. Once the NOTE is added to the Canto Lending Market, the creator of an asD token has the ability to withdraw the accumulated interest at any time.
• SLOC: 58

asDFactory

• Purpose: The asDFactory serves the purpose of generating new asD tokens. Its primary function, create, accepts the proposed name and symbol for the new token, which are not required to be unique. The factory maintains a record of all tokens it creates in the isAsD mapping, enabling other contracts to verify the authenticity of an asD token. This is particularly useful for those wanting to recognize all asD tokens, not just specific ones.
• SLOC: 22

asD

• Purpose: The application-specific dollar contract thats always to be pegged 1:1 with NOTE
• SLOC: 58

Minting Process

To mint asD tokens, the mint function is utilized. An equivalent amount of NOTE must be provided by the user, which is then allocated to the Canto Lending Market (CLM), effectively converting it into cNOTE.

Burning Mechanism

Utilizing the burn function, users can destroy their asD tokens. In exchange, they receive an equal amount of NOTE, which is initially retrieved from the CLM.

Interest Withdrawal

The withdrawCarry function allows the asD contract owner (typically the creator) to withdraw accrued interest. It's crucial that this function is designed to prevent the owner from withdrawing an excessive amount of tokens, ensuring the ability to redeem all asD tokens at their established 1:1 rate with NOTE.

One thing to give a great feedback on is the fact that contract uses whitelisting a lot, which is very commendable since this means that protocol only work with vetted parties, be it underlying tokens, share creators or what have you.

Important Links

• Previous audits: https://github.com/search?q=org%3Acode-423n4%20Canto-findings&type=code
• Documentation: Canto Docs would be helpful
• NOTE: Canto Unit of Account - USDNOTE
• Canto Lending Market: Overview
• Compound cTOKEN Documentation: Compound Finance
• Bonding curve's log2 logic: Copied from Solady: https://github.com/Vectorized/solady/blob/main/src/utils/FixedPointMathLib.sol

Testability

The system's testability is commendable being that is has a wide coverage, one thing to note would be the occasionally intricate lengthy tests that are a bit hard to follow, this shouldn't necessarily be considered a flaw though, cause once familiarized with the setup, devising tests and creating PoCs becomes a smoother endeavour. In that sense I'd commend the team for having provided a high-quality testing sandbox for implementing, testing, and expanding ideas.

Centralization Risks

As usual, like many other blockchain protocols, this one also relies significantly on the admin's integrity and non-malicious behaviour, any deviation from this could result in a range of issues, such as:

• The admin has the power to arbitrarily halt share creation or alter the permissible bonding curves, which could disrupt the entire share creation process, effectively disrupting the system
• A specific user could be targeted by the admin, who might manipulate the share creation process by modifying the whitelists in changeShareCreatorWhitelist() front running the user's execution, effectively barring the user from participating.
• There's a potential risk of a share creator embedding harmful metadata URI while setting up a new share.

Systemic Risks

• The system does not impose any caps on the number of shares that can be created, potentially leading to scalability challenges if a substantial number of shares are integrated into the system
• Absence of emergency stop functionalities could leave the system vulnerable in extreme market conditions or black swan events, and while such features could lead to further centralization, they are critical to consider.
• The idea of having a limit for the creator in buying tokens for shares they created is a fallacy, as this could easily be circumvented by using an alias address.

Summary of Reported Findings

During my security review, I identified several issues impacting the smart contract functionalities, each presenting unique challenges and potential risks to the system's overall integrity, one of which is the fact that in Market.sol, the lack of slippage protection in the sell() function exposes users to potential sandwich attacks. Attackers can exploit this by manipulating the market price before and after a user's transaction, leading to significant financial losses for the user. A recommended fix is to allow users to set a minimum acceptable sale price, thus ensuring their transaction is reverted if the market price is manipulated unfavourably.

The sell() function in Market.sol currently operates without a deadline, creating a risk of financial loss for users. If a transaction is executed later than intended, despite meeting the minimum price requirements, users might face unfavourable US dollar valuations. To mitigate this, it's recommended to introduce a user-defined deadline for each transaction within the sell function, providing users control over the timing and enhancing transactional security.

In LinearBondingCurve.sol, the fee calculation mechanism, which relies on the logarithm of share count, presents a significant flaw. This method leads to a disproportionately slow increase in fees compared to the growth in share count, as evidenced by provided graphical analyses, to put this in perspective a share count increase of 10,000% would only lead to a ~78% increase in fees. Such a fee structure risks becoming almost stagnant and suffers from precision loss, potentially resulting in insufficient fee allocation to holders, creators, and the platform, particularly as the protocol scales. Revising the fee calculation formula or addressing the precision loss is recommended to ensure fair and adequate fee distribution.

In multiple contracts in scope, like aSD.sol, there's an oversight in not checking the return value of the turnstile.register() call, particularly on the Canto mainnet or testnet (chain IDs 7700/7701). This omission could lead to unanticipated behaviours if the registration fails or the function reverts, potentially impacting the contract's operations. Enhancing this with a check and appropriate error handling would ensure more robust and predictable functionality.

Market.sol faces a denial-of-service risk for some users due to the use of hardcoded recipient addresses in its sell() function. If a user is blacklisted by the underlying ERC20 token, they become unable to sell their shares, leading to potential asset lock-up and financial losses in US$ value if market is very turbulent. To address this, it's advisable to modify the function to allow users to specify an alternative address for receiving sale proceeds, circumventing possible blacklisting issues.

The asD::withdrawCarry() function's integration with Compound's redeemUnderlying() is incorrectly referenced, pointing to #redeem in the documentation. This misalignment may lead to confusion or improper use of the Compound protocol. Adjusting the reference to correctly point to #redeemUnderlying, and ensuring the function aligns with the expected behaviour, will improve clarity and functionality.

Currently, the shareCount in the getPriceAndFee() function lacks a maximum limit, which could lead to out-of-gas errors during execution. Imposing a cap on shareCount will prevent such issues, ensuring smoother and more efficient operation, especially when dealing with large amounts of data or iterations.

The getNFTMintingPrice() function's documentation suggests it returns both price and fee, but the actual implementation returns only the fee. This discrepancy between the documentation and functionality can lead to confusion. Aligning the function with its documentation or revising the documentation to accurately reflect the function’s purpose will enhance understanding and usability.

The use of assembly code in functions like LinearBondingCurve::log2() without adequate commenting poses a risk due to the complex nature of such code. Detailed comments explaining the assembly logic would greatly aid in understanding and maintaining the code, reducing the likelihood of errors.

The lack of emergency functionalities across various in-scope contracts could leave the system vulnerable during unforeseen events or crises. Introducing mechanisms to pause or modify operations during emergencies would add a layer of security and control, albeit at the cost of some decentralization.

The fee calculation process within Market.sol is currently under-documented, leading to potential misunderstandings. Improved documentation detailing how fees are calculated and distributed will provide clarity for users and developers, ensuring a more transparent and user-friendly experience.

Learnt About

During the course of my security review, I had to research some stuffs to understand the context in which they are currently being used, some of which led to me learning new things and having refreshers on already acquainted topics. Some of these topics are listed below:

• A brief review of the Canto Lending Market.
• Canto's NOTE.
• Compund's cTOKEN.
• Had a refresher on ERC 1155.

Other Recommendations

Enhance Documentation Quality

There are currently some gaps concerning what's within the scope. It's important to remember that code comments also count as documentation, and there have been some inconsistencies in this area. Comprehensive documentation not only provides clarity but also facilitates the onboarding process for new developers. For instance, while withdrawCarrying the code documentation point to a completely different function than what's been queried from Compound.

Onboard More Developers

Having multiple eyes scrutinizing a protocol is always valuable. More contributors can significantly reduce potential risks and oversights. Evidence of where this could come in handy can be gleaned from codebase typos. Drawing from the broken windows theory, such lapses may signify underlying code imperfections.

Leverage Additional Auditing Tools

Many security experts prefer using Visual Studio Code augmented with specific plugins. While the popular Solidity Visual Developer has already been integrated with the protocol, there's room for incorporating other beneficial tools.

Improve Testability

Where as the previous comment on testing was heavily commending it, for any good thing it could definitely get better, and as such team should be trying to hit the 100% coverage.

Refine Naming Conventions

There's a need to improve the naming conventions for contracts, functions, and variables. In several instances, the names don't resonate with their respective functionalities, leading to potential confusion, an example of this can be seen with the getNFTMintingPrice() function, unlike it's name and the documentation attached to it, this function only returns the fee attached to minting the NFT.

Security Researcher Logistics

My attempt on reviewing the Codebase spanned around 12 hours distributed around 2 days:

• 1.5 hours dedicated to writing this analysis.
• 3 hours spent breezing through the previous CANTO contests on Code4rena.
• 1.5 hours over the course of the review were spent in the Discord chat to gain a deeper understanding of the protocol and gather insights from explanations provided by the protocol developers.
• The remaining time was spent on finding issues and writing the report for each of them on the spot, later on editing a few with more knowledge gained on the protocol as a whole, or downgrading them to QA reports.

Conclusion

The codebase was a very great learning experience, though it was a pretty hard nut to crack, being that it has a little amount of nSLOC and as usual the lesser the lines of code the lesser the bug ideas , nonetheless it was a great ride seeing the unique implementations employed in the codebase.

Time spent:

12 hours