Canto Invitational
Findings & Analysis Report

2024-05-03

Overview
- About C4
- Wardens
Summary
Scope
Severity Criteria
High Risk Findings (2)
- [H-01] Native gas tokens can become stuck in ASDRouter contract
- [H-02] Dual transaction nature of composed message transfer allows anyone to steal user funds
Medium Risk Findings (3)
Low Risk and Non-Critical Issues
Audit Analysis
Disclosures

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Canto smart contract system written in Solidity. The audit took place between March 29 — April 3, 2024.

Wardens

In Code4rena’s Invitational audits, the competition is limited to a small group of wardens; for this audit, 5 wardens contributed reports:

This audit was judged by 3docSec.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 5 unique vulnerabilities. Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity and 3 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 5 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Canto repository, and is composed of 3 smart contracts and 3 interfaces written in the Solidity programming language and includes 247 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

Malicious Input Handling
Escalation of privileges
Arithmetic
Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (2)

[H-01] Native gas tokens can become stuck in `ASDRouter` contract

Submitted by 0xTheC0der, also found by SpicyMeatball and oakcobalt

In case of a successful asD redemption flow, native gas tokens (CANTO) can become stuck in the ASDRouter contract.

A successful execution of lzCompose(…) ends with a call to _sendASD(…):

function _sendASD(bytes32 _guid, OftComposeMessage memory _payload, uint _amount) internal {
    /* transfer the ASD tokens to the destination receiver */
    if (_payload._dstLzEid == cantoLzEID) {
        // just transfer the ASD tokens to the destination receiver
        ...
        ASDOFT(_payload._cantoAsdAddress).transfer(_payload._dstReceiver, _amount);   // @audit 0 of msg.value used
    } else {
        // use Layer Zero oapp to send ASD tokens to the destination receiver on the destination chain

        // make sure msg.value is enough to cover the fee or this transaction will revert
        if (msg.value < _payload._feeForSend) {   // @audit msg.value >= _payload._feeForSend seems intended, otherwise != should be used
            // refund ASD tokens on canto
            _refundToken(_guid, _payload._cantoAsdAddress, _payload._cantoRefundAddress, _amount, msg.value, "insufficient msg.value for send fee");
            return;
        }

        // create send params for the Layer Zero oapp
        ...

        // send tokens
        (bool successfulSend, bytes memory data) = payable(_payload._cantoAsdAddress).call{value: _payload._feeForSend}(abi.encodeWithSelector(IOFT.send.selector, sendParams, fee, _payload._cantoRefundAddress));   // @audit only _payload._feeForSend of msg.value used

        // check if the send was successful
        if (!successfulSend) {
            // refund ASD tokens on canto
            _refundToken(_guid, _payload._cantoAsdAddress, _payload._cantoRefundAddress, _amount, msg.value, string(data));
            return;
        }
        ...
    }
    // @audit rest of msg.value is not refunded
}

Thereby, one can distinguish 3 scenarios:

On error: msg.value is fully refunded to specified refund address.
On successful transfer (within Canto chain): 0 of msg.value is used and any remaining value will be stuck.

One cannot assume and the contract doesn’t require that msg.value == 0 always holds in this case.

On successful transfer (to other chain): _payload._feeForSend of msg.value is used for sending asD, the remaining value will be stuck.

One cannot assume and the contract doesn’t require that msg.value == _payload._feeForSend always holds in this case.

Furthermore, the present behaviour violates a protocol invariant, according to the README:

ASDRouter native balance should always be zero.

Proof of Concept

The diff below modifies the existing test case lzCompose: successful deposit and send on canto to demonstrate that excess msg.value is not refunded in case of a successful asD redemption flow.

diff --git a/test/ASDRouter.js b/test/ASDRouter.js
index 2a36337..eccedc0 100644
--- a/test/ASDRouter.js
+++ b/test/ASDRouter.js
@@ -276,6 +276,7 @@ describe("ASDRouter", function () {
     it("lzCompose: successful deposit and send on canto", async () => {
         // update whitelist
         await ASDUSDC.updateWhitelist(USDCOFT.target, true);
+        const gas = ethers.parseEther("1");
         // call lzCompose with valid payload
         await expect(
             ASDRouter.lzCompose(
@@ -287,12 +288,18 @@ describe("ASDRouter", function () {
                     generatedRouterPayload(cantoLzEndpoint.id, refundAddress, TESTASD.target, TESTASD.target, "0", refundAddress, "0")
                 ),
                 executorAddress,
-                "0x"
+                "0x",
+                { value: gas }
             )
         )
             .to.emit(ASDRouter, "ASDSent")
             .withArgs(guid, refundAddress, TESTASD.target, amountUSDCSent, cantoLzEndpoint.id, false);
         // expect ASD to be sent to canto
         expect(await TESTASD.balanceOf(refundAddress)).to.equal(amountUSDCSent);
+        
+        // expect gas to be refunded and not held in ASDRouter
+        expect(await ethers.provider.getBalance(ASDRouter.target)).to.equal(0);
+        expect(await ethers.provider.getBalance(refundAddress)).to.equal(gas);
+
     });
 });

Recommended Mitigation Steps

Apply the following changes to the _sendASD(…) method in order to refund the remaining value (i.e. the native contract balance).

diff --git a/contracts/asd/asdRouter.sol b/contracts/asd/asdRouter.sol
index 7cb4ecc..37786e1 100644
--- a/contracts/asd/asdRouter.sol
+++ b/contracts/asd/asdRouter.sol
@@ -164,6 +164,10 @@ contract ASDRouter is IOAppComposer, Ownable {
             }
             emit ASDSent(_guid, _payload._dstReceiver, _payload._cantoAsdAddress, _amount, _payload._dstLzEid, true);
         }
+
+        if (address(this).balance > 0) {
+            payable(_payload._cantoRefundAddress).transfer(address(this).balance);
+        }
     }
 
     /**

Note: Using transfer might not be an ideal choice, but this proposed solution in meant to be in analogy with the _refundToken(...) method.

Assessed type

ETH-Transfer

3docSec (judge) commented:

Looks valid. The difference between msg.value and _payload._feeForSend is indeed not refunded to _payload._cantoRefundAddress as one would expect.

dsudit01 (Canto) confirmed

3docSec (judge) increased severity to High and commented:

Upgrading to High as if I am not mistaken, there is no viable way to safely recover the native tokens stuck in the contract.

SpicyMeatball (warden) commented:

@3docSec - it is true that there is no way to recover ETH from the router once the transaction succeeds, but I think it should be viewed as a user’s mistake and QA is more appropriate. The report describes two cases in which ETH can stuck:

On successful transfer (within Canto chain) - there is no need for the user to send a non-zero msg.value in this case.

On successful transfer (to other chain) - since _feeForSend is known beforehand, sending msg.value > _feeForSend can be considered a mistake on user’s side.

3docSec (judge) commented:

On successful transfer (within Canto chain).

Agree, Low impact there.

sending msg.value > _feeForSend can be considered a mistake on user’s side.

The logic is protected against excess tokens and has a refundAddress for this purpose. The fact that refunds cover only full refunds and not partial ones is not one a user could reasonably expect.

[H-02] Dual transaction nature of composed message transfer allows anyone to steal user funds

Submitted by 0xTheC0der, also found by d3e4 and oakcobalt

Sending OFTs with a composed message via LayerZero V2 involves 2 transactions on the destination chain (Canto) according to the documentation (see also README):

Receiving: The LayerZero endpoint receives the message and processes the OFT transfer to the ASDRouter contract.
Executing: Since the message is composed, it’s permissionlessly processed by an executor who invokes the lzCompose(…) method of the ASDRouter contract.

The above steps are processed in separate transactions (not atomic) and step 2 requires the OFTs to be already transferred to the ASDRouter contract.

Furthermore, due to the permissionless nature of the execution step, the lzCompose(…) method does not enforce any constraints on the msg.sender; therefore, allows anyone to invoke it.

Attack path:

An adversary can monitor the ASDRouter contract for whitelisted incoming USDC-like OFTs and immediately use those on their own (redeem asD) by crafting & invoking a call to lzCompose(…) before an executor does. As a consequence, user funds are directly stolen.

Similarly, if lzCompose(…) fails for any reason, an adversary can re-try before the user to use the OFTs.

Proof of Concept

The dual transaction nature of the current composed message process is confirmed by the LayerZero documentation:

If the message is composed, the contract retrieves and re-encodes the additional composed message information, then delivers the message to the endpoint, which will execute the additional logic as a separate transaction.

Moreover, crafting a successful call to lzCompose(…) once OFTs are deposited is demonstrated by the existing test case “lzCompose: successful deposit and send on canto”.

Recommended Mitigation Steps

Immediate, but not a satisfying fix: Restrict the lzCompose(…) method to trusted/whitelisted executors.

Alternative: Design change by directly implementing _lzReceive(…) (building an OApp) such that a composed message is not needed anymore.

Assessed type

Invalid Validation

3docSec (judge) commented:

Looks legitimate: lzCompose is permissionless and can be front-run because LayerZero calls it in a separate transaction after funds are delivered.

dsudit01 (Canto) confirmed and commented:

PR here.

Medium Risk Findings (3)

[M-01] `asdRouter.sol` is at risk of DOS due to vulnerable implementation of `NOTE` address

Submitted by oakcobalt

Proof of Concept

Based on Canto doc, NOTE address may be modified and redeployed:

The NOTE smart contract may be modified and redeployed at a new address in the future. For this reason, DApps should use the CToken.underlying() view on the cNOTE contract to determine the address for NOTE instead of hardcoding it.

As stated in doc, Dapp should use CToken.underlying() to ensure that NOTE address used is always correct.

The problem is current implementation in asdRouter.sol will only initiate noteAddress at contract deployment. noteAddress can not be changed afterward and the effect is the same as hardcoding an address.

//contracts/asd/asdRouter.sol
    constructor(address _noteAddress, uint32 _cantoLzEID, address _crocSwapAddress, address _crocImpactAddress, address _asdUSDCAddress) {
        //@audit No method to change noteAddress after deployment.
|>      noteAddress = _noteAddress;
        cantoLzEID = _cantoLzEID;
        crocSwapAddress = _crocSwapAddress;
        crocImpactAddress = _crocImpactAddress;
        asdUSDC = _asdUSDCAddress;
    }

https://github.com/code-423n4/2024-03-canto/blob/52dd3d7083ca9bed0484180f4a5f3af84a10a9bc/contracts/asd/asdRouter.sol#L45

Note: When performing swap from asdUSDC to NOTE, noteAddress will be used instead of CToken.underlying(). This means when NOTE address changes, swap and lzcompose will be DOSed.

//contracts/asd/asdRouter.sol
    function _swapOFTForNote(address _oftAddress, uint _amount, uint _minAmountNote) internal returns (uint, bool) {
...
        if (_oftAddress < noteAddress) {
            baseToken = _oftAddress;
|>          quoteToken = noteAddress;
        } else {
|>          baseToken = noteAddress;
            quoteToken = _oftAddress;
        }
...

https://github.com/code-423n4/2024-03-canto/blob/52dd3d7083ca9bed0484180f4a5f3af84a10a9bc/contracts/asd/asdRouter.sol#L214-L218

For comparison, asdOFT.sol has the correct NOTE address implementation where cNoteToken.underlying() is used.

Recommended Mitigation Steps

Change to store cNote address and only use CNote.underlying().

dsudit01 (Canto) confirmed and commented:

PR here.

SpicyMeatball (warden) commented:

@3docSec - I think QA is more appropriate:

The probability is low, changing the NOTE address is a rare event which may never occur.

The impact is medium at best, a temporary DOS which can be mitigated by redeploying the router with a new NOTE address. Canto can do it prior to redeploying the NOTE token to streamline the process.

3docSec (judge) commented:

The doc is clear about this, cNOTE address changes are a design choice in the ecosystem, and that’s a fact. That the event is rare and may never happen is subjective.

As per the second point, the SC helped clear out that the possibility of deploying a new version of the contract should not be reason for mitigating severity of a finding.

[M-02] `ASDRouter` didn’t call `ASDUSDC.approve()` to to grant permission for `crocSwapAddress` to spend their `ASDUSDC`

Submitted by 0xpiken, also found by SpicyMeatball

The function ‘ASDRouter#lzCompose()’ never succeeds. The receiver can only receive ASDUSDC token when the call of ‘ASDRouter#lzCompose()’ is done.

Proof of Concept

‘ASDRouter#lzCompose()’ is used to deposit wrapped USDC token for ASD token:

Once wrapped USDC is received, it will be deposited for ASDUSDC by calling ASDUSDC.deposit().
Next ASDUSDC will swapped to NOTE by calling _swapOFTForNote().
In the last, all NOTE will be deposited into ASD vault for ASD token by calling _depositNoteToASDVault().

However, in the second step, ASDUSDC.approve() was not invoked to authorize crocSwapAddress to spend their ASDUSDC. Consequently, crocSwapAddress#swap() cannot successfully swap ASDUSDC for NOTE. _swapOFTForNote() will fail and all ASDUSDC will be refunded to the designated receiver.

To demonstrate the aforementioned issue, the POC is divided into two parts:

The allowance that crocSwapAddress is allowed to spend from ASDRouter is 0. Modify asdRouter.sol as below:

...
import {ASDUSDC} from "./asdUSDC.sol";
+import {console} from "hardhat/console.sol";
...
    function _swapOFTForNote(address _oftAddress, uint _amount, uint _minAmountNote) internal returns (uint, bool) {
         ...
+        console.log("balance:", ASDUSDC(asdUSDC).balanceOf(address(this)));
+        console.log("allowance:", ASDUSDC(asdUSDC).allowance(address(this), crocSwapAddress));
        // swap is good to make, use call just in case revert occurs
        (bool successSwap, bytes memory data) = crocSwapAddress.call(abi.encodeWithSelector(ICrocSwapDex.swap.selector, baseToken, quoteToken, ambientPoolIdx, !isNoteBase, !isNoteBase, amountConverted, 0, 0, uintMinAmount, 0));
...

Run npx hardhat test. From the log we can see that the allowance is 0, while the ASDUSDC balance of ASDRouter is not:

      ASDRouter
        ✔ lzCompose: invalid payload
        ✔ lzCompose: invalid payload with gas
        ✔ lzCompose: not whitelisted
        ✔ lzCompose: not whitelisted with gas
        ✔ lzCompose: bad swap
        ✔ lzCompose: bad swap with gas
    balance: 100000000000000000000
    allowance: 0
        ✔ lzCompose: insufficient send fee
    balance: 100000000000000000000
    allowance: 0
        ✔ lzCompose: insufficient send fee with gas
    balance: 100000000000000000000
    allowance: 0
        ✔ lzCompose: successful deposit and send on canto

      ASDUSDC
        ✔ should set whitelist for correct contract addresses
        ✔ should deposit whitelisted usdc versions for asdUSDC
        ✔ should withdraw whitelisted usdc versions for asdUSDC
        ✔ should recover USDC tokens sent to the contract

crocSwapAddress#swap() wouldn’t work if the caller didn’t approve crocSwapAddress to spend their token. Add fork configuration into hardhat.config.js:

...
    networks: {
+       hardhat: {
+           forking: {
+             url: "https://canto-rpc.ansybl.io/",
+             blockNumber: 8977374,
+           },
+           chainId: 7700,
+       },
...

Create test/DEX.js:

const { expect } = require("chai");
const { ethers } = require("hardhat");
const hre = require("hardhat");

describe("Dex", function () {
    const dexAddress = "0x9290C893ce949FE13EF3355660d07dE0FB793618";
    const usdcAddress = "0x80b5a32E4F032B2a058b4F29EC95EEfEEB87aDcd";
    const cNoteAddress = "0xEe602429Ef7eCe0a13e4FfE8dBC16e101049504C";
    const usdcWhaleAddress = "0xfAc5EBD2b1b830806189FCcD0255DC9B174decbc";
    let dex;
    let usdc;
    let cNote;
    this.beforeEach(async () => {
        dex  = await hre.ethers.getContractAt("ICrocSwapDex", dexAddress);
        usdc = await hre.ethers.getContractAt("IERC20", usdcAddress);
        cNote = await hre.ethers.getContractAt("CErc20Interface", cNoteAddress);
    });

    it("User can only call DEX.swap() for cNote after call USDC.approve(dex)", async () => {
        await hre.network.provider.request({
            method: "hardhat_impersonateAccount",
            params: [usdcWhaleAddress],
        });
        whale = await ethers.getSigner(usdcWhaleAddress);
        let usdcBalanceBeforeSwap = await usdc.balanceOf(usdcWhaleAddress);
        await usdc.connect(whale).approve(dexAddress, 0);
        expect(await usdc.allowance(usdcWhaleAddress,dexAddress)).to.be.equal(0);
        //@audit-info swap failed due to zero allowance
        expect(dex.connect(whale).swap(
            usdcAddress,
            cNoteAddress,
            36000,
            true,
            true,
            2000000,
            0, 
            0,
            0, 
            0 
        )).to.be.reverted;
        //@audit-info user set the allowance for dex to 2000000
        await usdc.connect(whale).approve(dexAddress, 2000000);
        expect(await usdc.allowance(usdcWhaleAddress,dexAddress)).to.be.equal(2000000);
        swapTx = await dex.connect(whale).swap(
            usdcAddress, 
            cNoteAddress,
            36000,
            true,
            true, 
            2000000,
            0,
            ethers.parseEther("10000000000"),
            0,
            0
        );
        await swapTx.wait();
        let usdcBalanceAfterSwap = await usdc.balanceOf(usdcWhaleAddress);
        let usedUSDC = usdcBalanceBeforeSwap - usdcBalanceAfterSwap;
        //@audit-info swap succeeded and 2000000 of USDC was tranferred from user
        expect(usedUSDC).to.be.equal(2000000);
    });
});

Open one terminal to run npx hardhat node, then open another terminal to run npx hardhat test. It’s obvious that dex.swap() will fail if usdc.approve() is not called first.

Recommended Mitigation Steps

Set allowance to the maximum value from the beginning:

    constructor(address _noteAddress, uint32 _cantoLzEID, address _crocSwapAddress, address _crocImpactAddress, address _asdUSDCAddress) {
        noteAddress = _noteAddress;
        cantoLzEID = _cantoLzEID;
        crocSwapAddress = _crocSwapAddress;
        crocImpactAddress = _crocImpactAddress;
        asdUSDC = _asdUSDCAddress;
+       ASDUSDC(asdUSDC).approve(crocSwapAddress, type(uint).max);
    }

Assessed type

Context

dsudit01 (Canto) confirmed and commented:

PR here.

SpicyMeatball (warden) commented:

@3docSec - the sole purpose of the router contract is to exchange USDC to asD and send it to the source chain. The bug described in this report disrupts this functionality making the router unusable. Therefore, I believe a high severity is more appropriate.

3docSec (judge) commented:

A quick refresher on our severity categorization:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Should help clarify how this finding is a well-deserved Medium.

[M-03] Removing token from the whitelist may cause DoS due to limited USDC amount

Submitted by SpicyMeatball, also found by 0xTheC0der and oakcobalt

Users won’t be able to withdraw USDC tokens from asdUSDC.

Proof of Concept

Consider a situation where we have two whitelisted USDC versions 1 and 2:

    usdcBalances[usdc1] = 1000
    usdcBalances[usdc2] = 500
    asdUSDC total supply = 1500

After some time, asdUSDC owner removes usdc2 from the whitelist. It creates a situation where 1500 asdUSDC is only backed by 1000 usdc1 tokens, since it’s no longer possible to withdraw usdc2.

https://github.com/code-423n4/2024-03-canto/blob/main/contracts/asd/asdUSDC.sol#L52

    function withdraw(address _usdcVersion, uint256 _amount) external returns (uint256) {
        // check whitelist
>>      require(whitelistedUSDCVersions[_usdcVersion], "ASDUSDC: USDC version not whitelisted");
        // burn tokens
        _burn(msg.sender, _amount);
        // calculate amount to withdraw
        uint256 amountToWithdraw = _amount / (10 ** (this.decimals() - ERC20(_usdcVersion).decimals()));
        // check balance
        require(usdcBalances[_usdcVersion] >= amountToWithdraw, "ASDUSDC: Not enough USDC balance");
        // transfer USDC
        usdcBalances[_usdcVersion] -= amountToWithdraw;
        SafeERC20.safeTransfer(ERC20(_usdcVersion), msg.sender, amountToWithdraw);
        emit Withdrawal(_usdcVersion, amountToWithdraw);
        return amountToWithdraw;
    }

If former usdc2 holders withdraw their 500 asdUSDC for usdc1, there will be no usdc1 left for other users.

Recommended Mitigation Steps

Consider removing the require block from withdraw function.

- require(whitelistedUSDCVersions[_usdcVersion], "ASDUSDC: USDC version not whitelisted");

dsudit01 (Canto) confirmed and commented:

PR here.

Low Risk and Non-Critical Issues

For this audit, 5 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by 0xTheC0der received the top score from the judge.

The following wardens also submitted reports: oakcobalt, d3e4, 0xpiken, and SpicyMeatball.

[L-01] Use of `transfer(...)` for native gas token transfers is not future-proof

See _refundToken(…) method:

function _refundToken(bytes32 _guid, address _tokenAddress, address _refundAddress, uint _amount, uint _nativeAmount, string memory _reason) internal {
    ...
    // transfer native tokens to refund address and check that this value is less than or equal to msg.value
    if (_nativeAmount > 0 && _nativeAmount <= msg.value) {
        payable(_refundAddress).transfer(_nativeAmount);   // @audit use of transfer
    }
}

The transfer(...) function forwards a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example, EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction. See here.

Recommendation

Use call(...) to prevent potential gas issues.

[L-02] Use of `transfer(...)` for native gas token transfers can lead to `lzCompose(...)` revert

See _refundToken(…) method:

function _refundToken(bytes32 _guid, address _tokenAddress, address _refundAddress, uint _amount, uint _nativeAmount, string memory _reason) internal {
    ...
    // transfer native tokens to refund address and check that this value is less than or equal to msg.value
    if (_nativeAmount > 0 && _nativeAmount <= msg.value) {
        payable(_refundAddress).transfer(_nativeAmount);   // @audit throws error
    }
}

In case the _refundAddress is a contract (it’s not documented that it cannot be a contract), the transfer might revert (out of gas, cannot accept native token, logic error, etc.) which leads to OFTs being temporarily stuck in the contract.

Recommendation

Use call(...) instead and check the success return value. If it returns false, wrap the native gas tokens into an ERC-20 (see also WETH) and transfer them to the _refundAddress.

[L-03] Precision loss in `withdraw(...)` can lead to all `asdUDSC` being burned while not all `USDC` is withdrawn

See withdraw(…) method:

function withdraw(address _usdcVersion, uint256 _amount) external returns (uint256) {
    // check whitelist
    require(whitelistedUSDCVersions[_usdcVersion], "ASDUSDC: USDC version not whitelisted");
    // burn tokens
    _burn(msg.sender, _amount);
    // calculate amount to withdraw
    uint256 amountToWithdraw = _amount / (10 ** (this.decimals() - ERC20(_usdcVersion).decimals()));   // @audit precision loss
    // check balance
    require(usdcBalances[_usdcVersion] >= amountToWithdraw, "ASDUSDC: Not enough USDC balance");
    // transfer USDC
    usdcBalances[_usdcVersion] -= amountToWithdraw;
    SafeERC20.safeTransfer(ERC20(_usdcVersion), msg.sender, amountToWithdraw);
    emit Withdrawal(_usdcVersion, amountToWithdraw);
    return amountToWithdraw;
}

The asdUDSC token has 18 decimals. For example, if the USDC token has 6 decimals (see ERC20 token behaviors in scope), the burned _amount gets divided by 10**12 leading to a precision loss in the resulting amountToWithdraw.

Proof of Concept

The following test case, which should deposit and withdraw with differing decimals, demonstrates how all asdUDSC is burned while not all USDC4 (with 6 decimals) is withdrawn:

diff --git a/contracts/test-contracts/TestERC20.sol b/contracts/test-contracts/TestERC20.sol
index dccc64d..229d8c6 100644
--- a/contracts/test-contracts/TestERC20.sol
+++ b/contracts/test-contracts/TestERC20.sol
@@ -3,7 +3,19 @@ pragma solidity ^0.8.22;
 import {IERC20, ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 
 contract TestERC20 is ERC20 {
+
+    uint8 private _decimals;
+
     constructor(string memory _name, string memory _symbol) ERC20(_name, _symbol) {
+        _decimals = 18;
         _mint(msg.sender, 1000 ether);
     }
+
+    function decimals() public view override returns (uint8) {
+        return _decimals;
+    }
+
+    function setDecimals(uint8 _val) public {
+        _decimals = _val;
+    }
 }
diff --git a/test/ASDUSDC.js b/test/ASDUSDC.js
index 62e66b8..5a100b0 100644
--- a/test/ASDUSDC.js
+++ b/test/ASDUSDC.js
@@ -6,6 +6,7 @@ describe("ASDUSDC", function () {
     let usdc1;
     let usdc2;
     let usdc3;
+    let usdc4;
     this.beforeEach(async () => {
         // deploy ASDUSDC contract
         asdUSDC = await ethers.deployContract("ASDUSDC");
@@ -13,9 +14,13 @@ describe("ASDUSDC", function () {
         usdc1 = await ethers.deployContract("TestERC20", ["USDC1", "USDC1"]);
         usdc2 = await ethers.deployContract("TestERC20", ["USDC2", "USDC2"]);
         usdc3 = await ethers.deployContract("TestERC20", ["USDC3", "USDC3"]);
-        // set whitelist for 2 of the tokens
+        usdc4 = await ethers.deployContract("TestERC20", ["USDC4", "USDC4"]);
+        await usdc4.setDecimals(6);
+
+        // set whitelist for 3 of the tokens
         await asdUSDC.updateWhitelist(usdc1.target, true);
         await asdUSDC.updateWhitelist(usdc2.target, true);
+        await asdUSDC.updateWhitelist(usdc4.target, true);
     });
 
     it("should set whitelist for correct contract addresses", async () => {
@@ -43,6 +48,26 @@ describe("ASDUSDC", function () {
         await expect(asdUSDC.deposit(usdc3.target, 1000)).to.be.revertedWith("ASDUSDC: USDC version not whitelisted");
     });
 
+    it("should deposit & withdraw with differing decimals", async () => {
+        const [signer] = await ethers.getSigners();
+        const usdc4Deposit = 1000e6;
+        await usdc4.approve(asdUSDC.target, usdc4Deposit);
+        await asdUSDC.deposit(usdc4.target, usdc4Deposit);
+
+        const usdc4Withdraw = BigInt(1000e18);
+        expect(await asdUSDC.balanceOf(signer)).to.equal(usdc4Withdraw);
+
+        //split withdrawal in 2 transactions to provoke rounding error
+        await asdUSDC.withdraw(usdc4.target, usdc4Withdraw / BigInt(2) - BigInt(1));
+        await asdUSDC.withdraw(usdc4.target, usdc4Withdraw / BigInt(2) + BigInt(1));
+
+        // check that all of user's asdUDSC is burned
+        expect(await asdUSDC.balanceOf(signer)).to.equal(0);
+
+        expect(await usdc4.balanceOf(asdUSDC.target), "asdUSDC contract still has USDC4").to.equal(0);
+
+    });
+
     it("should withdraw whitelisted usdc versions for asdUSDC", async () => {
         const [signer] = await ethers.getSigners();
         const usdc1Deposit = 1000;

Recommendation

Re-calculate the actual _amount to burn by upscaling from amountToWithdraw to avoid burning more than what is actually withdrawn.

[L-04] The `asdUSDC` contract insufficiently supports `USDC` tokens with more than 18 decimals, resulting in a DoS scenario

Instances: deposit(…), withdraw(…), recover(…).

...
uint256 amountTo... = _amount * (10 ** (this.decimals() - ERC20(_usdcVersion).decimals()));
...

The asdUDSC token has 18 decimals. In case of a USDC token having more than 18 decimals (see ERC20 token behaviors in scope), the code above will revert due to an “arithmetic underflow” error (of uint8), which effectively causes DoS of the asdUSDC contract for such tokens.

Recommendation

Also covers the case when ERC20(_usdcVersion).decimals() > 18:

...
if (this.decimals() >= ERC20(_usdcVersion).decimals()) {
    amountTo... = _amount * (10 ** (this.decimals() - ERC20(_usdcVersion).decimals()));
}
else {
    amountTo... = _amount / (10 ** (ERC20(_usdcVersion).decimals() - this.decimals()));
}
...

[L-05] The `asdUSDC` contract can be used as a free DEX

Due to the permissionless and feeless nature of the deposit(…) and withdraw(…) methods, everyone can use them to swap whitelisted USDC-like tokens for free. This might be especially attractive (for arbitraging) if such a stablecoin slightly depegs.

Consequence

Users cannot expect to be able to withdraw the same stablecoin they’ve deposited since its underlying balance might be depleted.

Recommendation

Restrict the deposit(…) method to only be callable from the asdRouter contract.

[L-06] The `asdUSDC` owner can block withdrawals by removing a token from the whitelist

See withdraw(…):

function withdraw(address _usdcVersion, uint256 _amount) external returns (uint256) {
    // check whitelist
    require(whitelistedUSDCVersions[_usdcVersion], "ASDUSDC: USDC version not whitelisted");  // @audit owner can block withdrawals
    // burn tokens
    _burn(msg.sender, _amount);
    // calculate amount to withdraw
    uint256 amountToWithdraw = _amount / (10 ** (this.decimals() - ERC20(_usdcVersion).decimals()));
    // check balance
    require(usdcBalances[_usdcVersion] >= amountToWithdraw, "ASDUSDC: Not enough USDC balance");
    // transfer USDC
    usdcBalances[_usdcVersion] -= amountToWithdraw;
    SafeERC20.safeTransfer(ERC20(_usdcVersion), msg.sender, amountToWithdraw);
    emit Withdrawal(_usdcVersion, amountToWithdraw);
    return amountToWithdraw;
}

If the owner of asdUSDC decides to stop supporting an USDC-like token and therefore, proceeds to remove it from the whitelist, withdrawals will also be disabled leading to locked user value.

Recommendation

Remove the whitelist check from the withdraw(…) method; it’s sufficient to enforce it within deposit(…). Therefore, once deposited, USDC-like tokens can always be withdrawn.

[L-07] Fixed execution gas amount might not work for all potential target chains

In _sendASD(…), when asD is transferred to another chain via LayerZero, a fixed gas amount (in native tokens of the source chain, see also documentation) of 200000 is provided for the execution of lzReceive(...) (on the destination chain).

// create send params for the Layer Zero oapp
bytes memory sendOptions = OptionsBuilder.addExecutorLzReceiveOption(OptionsBuilder.newOptions(), 200000, 0);

At the time of writing, 200000 canto * 0.258 $CANTO = 5.16e-11 $ which might be insufficient to cover the gas fees for the execution of lzReceive(...) on most destination chains.

Recommendation

Compute this value according to the destination chain or let the user specify it similar to _payload._feeForSend. Also, the quoting mechanism can help with that.

dsudit01 (Canto) commented:

The message from lzCompose is a hardcoded length from the OFT contract itself. We can still check the length to be sure though.

PR here.

Note: For full discussion, see here.

Audit Analysis

For this audit, 1 analysis report was submitted by wardens. An analysis report examines the codebase as a whole, providing observations and advice on such topics as architecture, mechanism, or approach. The report highlighted below by oakcobalt received the top score from the judge.

Summary

asD enables protocols to earn yield on user dollar deposits, always pegged to 1 $NOTE. Depositing $NOTE into the Canto Lending Market accrues interest. This version of asD utilizes LayerZero’s Omnichain Fungible Token, enabling creators from any LayerZero supported chain to earn extra revenue from stable coin deposits.

Existing standards

ERC20 token (including fee-on-transfer, low/high decimal behaviors).

Approach:

Scope: CTokenInterfaces.sol, Turnstile.sol, asdOFT.sol, asdRouter.sol, asdUSDC.sol, CrocInterfaces.sol. (3 interfaces + 3 contracts). My focus is on the three contracts.
Roles: Role-specific flows are focused including onlyOwner flows, user flows and executor flows.
Layer zero protocol: layer zero related OFT features are evaluated as context to analyze any vulnerable implementations in scope.

Centralization Risks

Here’s an analysis of potential centralization risks in the provided contracts:

`asdOFT.sol`

As intended design, only owner of the contract can withdraw yield from CToken contract. Regular users are not able to claim yield.

Overall low centralization risk.

`asdRouter.sol`

Although the contract has owner, low centralization risks - due to main logics and public-facing functions are not under the owner’s control (no onlyOwner modifier and no centralized parameters).

`asdUSDC.sol`

Higher centralization risk due to the USDC whitelisting and delisting features through updateWhitelist.

Systemic Risks

`CToken` liquidation risk

asdOFT.sol relies on minting and redeeming on CNOTE token. CToken is essentially a lending/borrow market with inherent liquidation risks. Although low risk for depositors, it’s possible that any case of liquidation failure during extreme market conditions or other factors, depositors cannot fully redeem their funds. This will affect ASD holders.

//contracts/asd/asdOFT.sol
    function mint(uint256 _amount) external {
...
        uint256 returnCode = cNoteToken.mint(_amount); 
...
    }
    function burn(uint256 _amount) external {
...
      uint256 returnCode = cNoteToken.redeemUnderlying(_amount); // Request _amount of NOTE (the underlying of cNOTE) 
...

https://github.com/code-423n4/2024-03-canto/blob/52dd3d7083ca9bed0484180f4a5f3af84a10a9bc/contracts/asd/asdOFT.sol#L38-L57

`CrocSwapDex` market risk

asdRouter.sol relies on CrocSwapDex market to swap USDC for NOTE. Like any dex, market volatility might cause price ratio changes or other unexpected market pause or failures which will impact the lzCompose tx, causing tx fail. Although refunds will be issued for users, any upfront fees user on the source chain might have paid will be lost.

//contracts/asd/asdRouter.sol
    function _swapOFTForNote(address _oftAddress, uint _amount, uint _minAmountNote) internal returns (uint, bool) {
...
        // swap is good to make, use call just in case revert occurs
        (bool successSwap, bytes memory data) = crocSwapAddress.call(abi.encodeWithSelector(ICrocSwapDex.swap.selector, baseToken, quoteToken, ambientPoolIdx, !isNoteBase, !isNoteBase, amountConverted, 0, 0, uintMinAmount, 0));
...

https://github.com/code-423n4/2024-03-canto/blob/52dd3d7083ca9bed0484180f4a5f3af84a10a9bc/contracts/asd/asdRouter.sol#L247

Mechanism Review

Swapping implementation

In asdRouter.sol, swapping asdUSDC for NOTE will be performed for every lzCompose. Current swapping implementation might be wasteful in that it performs extra steps to check baseToken, quoteToken, sorting, and verifying pool address even though the token and pool should have already been known in contract deployment.

//contracts/asd/asdRouter.sol
    function _swapOFTForNote(address _oftAddress, uint _amount, uint _minAmountNote) internal returns (uint, bool) {
        // sort tokens
        address baseToken;
        address quoteToken;
        if (_oftAddress < noteAddress) {
            baseToken = _oftAddress;
            quoteToken = noteAddress;
        } else {
            baseToken = noteAddress;
            quoteToken = _oftAddress;
        }
        // check if pool exists
        //@audit-info note: in the context of lzCompose, baseToken and quoteToken will always be asdUSDC and $NOTE
        if (ambientPoolFor(baseToken, quoteToken, ambientPoolIdx) == 0) {
            // nothing swapped, swapped failed
            return (0, false);
        }
...
        bool isNoteBase = baseToken == noteAddress;
...
        if (isNoteBase && -baseFlow < minAmountInt) {
            // nothing swapped, swapped failed
            return (0, false);
        } else if (!isNoteBase && -quoteFlow < minAmountInt) {
            // nothing swapped, swapped failed
            return (0, false);
        }
...

https://github.com/code-423n4/2024-03-canto/blob/52dd3d7083ca9bed0484180f4a5f3af84a10a9bc/contracts/asd/asdRouter.sol#L209

Recommendations

The above checking, sorting base and quoteToken address and the if control flows might be redundant, given that base and quoteToken are fixed tokens, and their pool address should have been known at contract deployment. Consider refactoring swapping to allow token sorting and pool checking to be done at contract deployment. This will reduce runtime gas cost as well.

Conclusions

The report evaluates potential centralization risks within the provided contracts; noting low risks in asdOFT.sol and asdRouter.sol, while highlighting higher risks in asdUSDC.sol due to USDC whitelisting and delisting features. Furthermore, systemic risks such as CToken liquidation risk and market risks associated with CrocSwapDex are analyzed, underscoring potential implications for depositors and users. Mechanism reviews pinpoint areas for optimization, particularly in the swapping implementation of asdRouter.sol, suggesting refactoring to enhance efficiency and reduce runtime gas costs.

Overall, the report provides valuable insights into the protocol’s functionality, associated risks, and opportunities for improvement, laying a foundation for informed decision-making and potential protocol enhancements.

Time spent

32 hours

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.

Canto Invitational Findings & Analysis Report

2024-05-03

Table of contents

Overview

About C4

Wardens

Summary

Scope

Severity Criteria

High Risk Findings (2)

[H-01] Native gas tokens can become stuck in ASDRouter contract

Proof of Concept

Recommended Mitigation Steps

Assessed type

[H-02] Dual transaction nature of composed message transfer allows anyone to steal user funds

Proof of Concept

Recommended Mitigation Steps

Assessed type

Medium Risk Findings (3)

[M-01] asdRouter.sol is at risk of DOS due to vulnerable implementation of NOTE address

Proof of Concept

Recommended Mitigation Steps

[M-02] ASDRouter didn’t call ASDUSDC.approve() to to grant permission for crocSwapAddress to spend their ASDUSDC

Proof of Concept

Recommended Mitigation Steps

Assessed type

[M-03] Removing token from the whitelist may cause DoS due to limited USDC amount

Proof of Concept

Recommended Mitigation Steps

Low Risk and Non-Critical Issues

[L-01] Use of transfer(...) for native gas token transfers is not future-proof

Recommendation

[L-02] Use of transfer(...) for native gas token transfers can lead to lzCompose(...) revert

Recommendation

[L-03] Precision loss in withdraw(...) can lead to all asdUDSC being burned while not all USDC is withdrawn

Proof of Concept

Recommendation

[L-04] The asdUSDC contract insufficiently supports USDC tokens with more than 18 decimals, resulting in a DoS scenario

Recommendation

[L-05] The asdUSDC contract can be used as a free DEX

Consequence

Recommendation

[L-06] The asdUSDC owner can block withdrawals by removing a token from the whitelist

Recommendation

[L-07] Fixed execution gas amount might not work for all potential target chains

Recommendation

Audit Analysis

Summary

Existing standards

Approach:

Centralization Risks

asdOFT.sol

asdRouter.sol

asdUSDC.sol

Systemic Risks

CToken liquidation risk

CrocSwapDex market risk

Mechanism Review

Swapping implementation

Recommendations

Conclusions

Time spent

Disclosures

Canto Invitational
Findings & Analysis Report

[H-01] Native gas tokens can become stuck in `ASDRouter` contract

[M-01] `asdRouter.sol` is at risk of DOS due to vulnerable implementation of `NOTE` address

[M-02] `ASDRouter` didn’t call `ASDUSDC.approve()` to to grant permission for `crocSwapAddress` to spend their `ASDUSDC`

[L-01] Use of `transfer(...)` for native gas token transfers is not future-proof

[L-02] Use of `transfer(...)` for native gas token transfers can lead to `lzCompose(...)` revert

[L-03] Precision loss in `withdraw(...)` can lead to all `asdUDSC` being burned while not all `USDC` is withdrawn

[L-04] The `asdUSDC` contract insufficiently supports `USDC` tokens with more than 18 decimals, resulting in a DoS scenario

[L-05] The `asdUSDC` contract can be used as a free DEX

[L-06] The `asdUSDC` owner can block withdrawals by removing a token from the whitelist

`asdOFT.sol`

`asdRouter.sol`

`asdUSDC.sol`

`CToken` liquidation risk

`CrocSwapDex` market risk