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

Lines of code

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

Vulnerability details

Impact

The current withdrawCarry function in the contract underestimates the accrued interest due to a miscalculation. This error prevents the rightful owner from withdrawing their accrued interest, effectively locking the assets. The primary issue lies in the calculation of maximumWithdrawable within withdrawCarry.

Flawed Calculation:

The following code segment is used to determine maximumWithdrawable:

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

The problem arises with the scaling of exchangeRate, which is assumed to be scaled by 10^28. However, for CNOTE in the Canto network, the exchangeRate is actually scaled by 10^18. This discrepancy causes the owner to withdraw only 10^(-10) times the actual interest amount. Consequently, when a non-zero _amount is specified, withdrawCarry often fails due to the require(_amount <= maximumWithdrawable, "Too many tokens requested"); condition.

Proof of Concept

CNOTE Scaling Verification

An essential aspect of this audit involves verifying the scaling factor of the CNOTE exchange rate. The exchangeRate scale for CNOTE can be verified in the Canto Network's GitHub repository. Evidence confirming that the exponent of the CNOTE exchange rate is indeed 18 can be found through this link to the token tracker. The data provided there shows the current value of the stored exchange rate (exchangeRateStored) as approximately 1004161485744613000. This value corresponds to 1.00416 * 1e18, reaffirming the 10^18 scaling factor.

This information is critical for accurately understanding the mechanics of CNOTE and ensuring that the smart contract's calculations align with the actual scaling used in the token's implementation. The verification of this scaling factor supports the recommendations for adjusting the main contract's calculations and the associated test cases, as previously outlined.

Testing with Solidity Codes

Testing with the following Solidity code illustrates the actual CNOTE values:

function updateBalance() external {
    updatedUnderlyingBalance = ICNoteSimple(cnote).balanceOfUnderlying(msg.sender);
    updatedExchangeRate = ICNoteSimple(cnote).exchangeRateCurrent();

    uint256 balance = IERC20(cnote).balanceOf(msg.sender);
    calculatedUnderlying = balance * updatedExchangeRate / 1e28;
}

The corresponding TypeScript logs show a clear discrepancy between the expected and calculated underlying balances,

console.log("balanceCnote: ", (Number(balanceCnote) / 1e18).toString());
console.log("exchangeRate: ", Number(exchangeRate).toString());
console.log("underlyingBalance: ", Number(underlyingBalance).toString());
console.log("calculatedUnderlying: ", Number(calculatedUnderlying).toString());

with the logs

balanceCnote:  400100.9100006097
exchangeRate:  1004122567006264000
underlyingBalance:  4.017503528113544e+23
calculatedUnderlying:  40175035281135

Tools Used

• Solidity for interacting with the Canto mainnet.
• TypeScript for testing and validation.

Recommended Mitigation Steps

Using balanceOfUnderlying Function

Replace the flawed calculation with the balanceOfUnderlying function. This function accurately calculates the underlying NOTE balance and is defined in CToken.sol (source).

Proposed Code Modifications: Two alternative implementations are suggested:

1. Without balanceOfUnderlying: Modify the scaling factor in the existing calculation from 1e28 to 1e18.

function withdrawCarry(uint256 _amount) external onlyOwner {
    uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent(); // Scaled by 10^18
    // 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) /
        1e18 -
        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);
}

2. With balanceOfUnderlying (Recommended): Utilize the balanceOfUnderlying function for a simpler calculation of maximumWithdrawable.

function withdrawCarry(uint256 _amount) external onlyOwner {
    // 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).balanceOfUnderlying(address(this)) - 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 second option is highly recommended for its accuracy and simplicity.

Modification of Related Test Codes

Post-modifications to the main contract code, it's essential to update the associated test cases. In the MockCNOTE test contract, all scaling is currently set to 10^28. To align with the main contract changes, the following modifications are recommended for MockCNOTE:

contract MockCNOTE is MockERC20 {
    address public underlying;
    uint256 public constant SCALE = 1e18;
    uint256 public exchangeRateCurrent = SCALE;

    constructor(string memory symbol, string memory name, address _underlying) MockERC20(symbol, name) {
        underlying = _underlying;
    }

    function mint(uint256 amount) public returns (uint256 statusCode) {
        SafeERC20.safeTransferFrom(IERC20(underlying), msg.sender, address(this), amount);
        _mint(msg.sender, (amount * SCALE) / exchangeRateCurrent);
        statusCode = 0;
    }

    function redeemUnderlying(uint256 amount) public returns (uint256 statusCode) {
        SafeERC20.safeTransfer(IERC20(underlying), msg.sender, amount);
        _burn(msg.sender, (amount * exchangeRateCurrent) / SCALE);
        statusCode = 0;
    }

    function redeem(uint256 amount) public returns (uint256 statusCode) {
        SafeERC20.safeTransfer(IERC20(underlying), msg.sender, (amount * exchangeRateCurrent) / SCALE);
        _burn(msg.sender, amount);
        statusCode = 0;
    }

    function setExchangeRate(uint256 _exchangeRate) public {
        exchangeRateCurrent = _exchangeRate;
    }
}

This revised MockCNOTE contract reflects the updated scale factor and aligns with the main contract's logic.

Scenario Testing with Mainnet Forking

For comprehensive validation, scenario testing using a fork of the mainnet is highly recommended. This approach allows for real-world testing conditions by simulating interactions with existing contracts on the mainnet. It provides a robust environment to verify the correctness and reliability of the contract modifications in real-world scenarios, ensuring that the contract behaves as expected when interfacing with other mainnet contracts.

This step is crucial to identify potential issues that might not be apparent in isolated or simulated environments, enhancing the overall reliability of the contract before deployment.

Assessed type

Math

List of Findings

• 1155tech-contracts
  • Use Transfer(or TransferFrom) once not twice in a function
• asD contracts
  • Use Note address directly from storage not from deriving

1155tech-contracts

Use Transfer(or TransferFrom) once not twice in a function

In function buy(uint256 _id, uint256 _amount) external of Market.sol, token transfers are used twice as

SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee);

and

if (rewardsSinceLastClaim > 0) {
     SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim);
}

Instead of using transfer twice, the following can be used.

if (rewardsSinceLastClaim == 0) {
    SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee);
} else if (rewardsSinceLastClaim < price + fee) {
    SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee - rewardsSinceLastClaim);
} else if (rewardsSinceLastClaim > price + fee) {
    SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim - price - fee);
}

Then, the entire code of the function is as

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
    // The reward calculation has to use the old rewards value (pre fee-split) to not include the fees of this buy
    // The rewardsLastClaimedValue then needs to be updated with the new value such that the user cannot claim fees of this buy
    uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);

    if (rewardsSinceLastClaim == 0) {
        SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee);
    } else if (rewardsSinceLastClaim < price + fee) {
        SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee - rewardsSinceLastClaim);
    } else if (rewardsSinceLastClaim > price + fee) {
        SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim - price - fee);
    }

    // Split the fee among holder, creator and platform
    _splitFees(_id, fee, shareData[_id].tokensInCirculation);
    rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

    shareData[_id].tokenCount += _amount;
    shareData[_id].tokensInCirculation += _amount;
    tokensByAddress[_id][msg.sender] += _amount;

    emit SharesBought(_id, msg.sender, _amount, price, fee);
}

This reduces the gas of testBuy() from 351453 to 351441. This reduction becomes large when there are many buys and sells for a given id. The same thing can be applied to mintNFT and burnNFT functions, where the gas reductions of testMint() and testBurn are from 399055 and 347372, to 395283 and 347489, respectively. That is more impact than buy function.

asD contracts

Use Note address directly from storage not from deriving

In mint and burn functions of asD.sol, Note address is derived from cNoteToken.underlying() in the following code.

IERC20 note = IERC20(cNoteToken.underlying());

Instead, Note address can be stored and used for mint and burn. In order to do it, the following code modification is needed.

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

function mint(uint256 _amount) external {
    require(_amount > 0, "Cannot mint 0 tokens");
    CErc20Interface cNoteToken = CErc20Interface(cNote);
    IERC20 note = IERC20(Note);
    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);
}

/// @notice Burn amount of asD tokens to get back NOTE. Like when minting, the NOTE:asD exchange rate is always 1:1
/// @param _amount Amount of tokens to burn
function burn(uint256 _amount) external {
    require(_amount > 0, "Cannot burn 0 tokens");
    CErc20Interface cNoteToken = CErc20Interface(cNote);
    IERC20 note = IERC20(Note);
    uint256 returnCode = cNoteToken.redeemUnderlying(_amount); // Request _amount of NOTE (the underlying of cNOTE)
    require(returnCode == 0, "Error when redeeming"); // 0 on success: https://docs.compound.finance/v2/ctokens/#redeem-underlying
    _burn(msg.sender, _amount);
    SafeERC20.safeTransfer(note, msg.sender, _amount);
}

It can be seen that, the address of Note is stored in constructor and use it in mint and burn functions. Also, the following code modification is needed in asDFactory.sol.

address public immutable Note;
...
constructor(address _cNote, address _Note) {
    cNote = _cNote;
    Note = _Note;
    if (block.chainid == 7700 || block.chainid == 7701) {
        // Register CSR on Canto main- and testnet
        Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);
        turnstile.register(tx.origin);
    }
}

function create(string memory _name, string memory _symbol) external returns (address) {
    asD createdToken = new asD(_name, _symbol, msg.sender, cNote, Note, owner());
    isAsD[address(createdToken)] = true;
    emit CreatedToken(address(createdToken), _symbol, _name, msg.sender);
    return address(createdToken);
}

This reduces the gas for testBurn(), testMint(), testWithdrawCarry() from 263875, 214044, 272123 to 262556, 213391, 270786, respectively.