search

code-423n4 / 2023-11-canto-findings

7 stars 6 forks source link

Owner cannot withdraw all interest due to wrong calculation of accrued interest in WithdrwaCarry #181

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

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

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

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #227

c4-judge commented 1 year ago

MarioPoneder marked the issue as satisfactory

MarioPoneder commented 1 year ago

Selected for report due to overall discussion, PoC and mitigation

c4-judge commented 1 year ago

MarioPoneder marked the issue as selected for report

OpenCoreCH commented 1 year ago

Fixed in https://github.com/mkt-market/asD/pull/1