Solmate safeTransferLib.sol functions does not check the codesize of the token address, which may lead to fund loss

[hats-bug-reporter[bot]]

Github username: @0xRizwann Twitter username: 0xRizwann Submission hash (on-chain): 0x71c360d0e976cf6143c689dc503016878ff8709f92991c878e5bf3e67073314c Severity: medium

Description: Description

In CatalystFactory.sol contract, deployVault function is used to deploy the catalyst vault and then it funds the vault with tokens. The function has used safeTransferFrom from solmate's safeTransferLib.sol to send the tokens to vault.

        for (uint256 it; it < assets.length;) {
            ERC20(assets[it]).safeTransferFrom(    
                msg.sender,
                vault,
                init_balances[it]
            );

The used safeTransferFrom() function from solmate library which doesn't check the existence of code at the token address. This is a known issue while using solmate's libraries.

Per the Solmate safeTransferLib.sol,

Note that none of the functions in this library check that a token has code at all! .....

Hence using safeTransferLib.sol library may lead to miscalculation of funds and may lead to loss of funds , because if safeTransferFrom() are called on a token address that doesn't have contract in it, it will always return success, bypassing the return value check. Due to this protocol will think that funds has been transferred to vault by vault deployer and the transaction is successful , and records will be accordingly calculated, but in reality funds were never been transferred.

safeTransferFrom() function under the hood uses low level call function which can be checked here

However, solidity documentation strictly warns that,

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

code existence must be checked especially for low level functions like call,staticcall and delegatecall.

Openzeppelin and Solady confirms this and comply this requirements in their library, only solmate does not check code existence.

Recommendation

Recommended to use openzeppelin's safeERC20 which takes care of token code existence.

[0xfuje]

Damn, you were faster :D

[0xRizwan]

Similar issue instance in CatalystVaultAmplified.sol which has also used safeTransferLib.sol functions at L-381, L-605, L-770, L-825 and L-826, L-901 and L-1098

Another instance of issue in CatalystVaultCommon.sol and CatalystVaultVolatile.sol

It seems the solmate's safeTransferLib.sol is extensively used in different contracts. To be noted, this is well known issue with this libarary.

Openzeppelin and Solady confirms this and comply this requirements in their library, only solmate does not check code existence.

[reednaa]

You can't deploy a vault such that the vault has 0 balance of that token since that would break the mathematics.

Volatile: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultVolatile.sol#L113

Amplified: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultAmplified.sol#L136

[reednaa]

Correct me if I am wrong, this also invalidates all of the other "similar issue" since you cannot get there without somehow deploying a contract between the transfer and the check.

[0xRizwan]

Vault issue is one of the instance affected with this issue. I would request to check this comment. This is very much applicable here since at most of the functions token address/asset address is passed as argument and the above descriptions explains as above.

Reputed audit firms like code4rena, sherlock has confirmed such issue with Medium severity. Please check the Reference-1, Reference-2,

[reednaa]

How would you get in there with such a bug? A token that does a self-destruct?

But at that point, isn't something more severe already happening that we have no control over?

[reednaa]

Provide me with a PoC of you:

1. Creating a vault with the bug.
2. Swapping to the (non-existing) asset.

[0xRizwan]

Hi @reednaa,

I will provide additional context as below for the better understanding. I am heavily referencing this high severity issue which is an exactly similar issue with creation of vault with solmate safeTransferLib.sol which can be used to create fake balances with non-existing ERC20 tokens.

    function deployVault(
        address vaultTemplate,
        address[] calldata assets,
        uint256[] calldata init_balances,
        uint256[] calldata weights,
        uint256 amp,
        uint256 vaultFee,
        string memory name,
        string memory symbol,
        address chainInterface
    ) override external returns (address) {

     . . . some code

        // Create a minimal transparent proxy:
        address vault = Clones.clone(vaultTemplate);

        // The vault expects the balances to exist in the vault when setup is called.
        for (uint256 it; it < assets.length;) {
            ERC20(assets[it]).safeTransferFrom(      @audit // fake balances will be transferred for non-existing ERC20 tokens to vault
                msg.sender,
                vault,
                init_balances[it]
            );

     . . . some code

    }

deployVault() can be called by any user to deploy the Catalyst vault. The vault takes maximum 3 Assets as an argument and then transfer the funds to the vault with tokens and after that it calls the setup to configure the vaults.

These 3 Assets which are MAX_ASSETS for the Catalyst vault does not check the code existence of ERC20 token as it uses solmate safeTransferLib and this library has known issue and had been used to create non-existing ERC20 tokens with fake balances to rug the users later.

Due to non presence of code existing check, the safeTransferFrom while sending the tokens to vault, the transaction will succeed with no error.

This attack vector was made well-known by the qBridge hack back in Jan 2022.

With the rising use of crypto, it's becoming popular for protocols to deploy their token across multiple networks and when they do so, a common practice is to deploy the token contract from the same deployer address and with the same nonce so that the token address can be the same for all the networks.

For example: 1) $1INCH is using the same token address for both Ethereum and BSC. 2) $GEL token is using the same token address for Ethereum, Fantom and Polygon.

A sophisticated attacker can exploit it by taking advantage of that and setting traps on multiple potential tokens to steal from the future users that deposits with such tokens.

Proof of Concept: Consider a scenario,

• ProjectA has TokenA on another network;
• ProjectB has TokenB on another network;
• ProjectC has TokenC on another network;

1) Bob(attacker) calls the deployVault() for TokenA, TokenB, and TokenC with 10000e18 as init_balances each, here init_balances is the the initial balances of the vault which is already approved before transfer. TokenA, TokenB, and TokenC is considered in our case since only 3 Assets can be added during deploying of vault.

2) A few months later, ProjectB launched TokenB on the local network at the same address.(Presume TokenB had been launched with same token address earlier on other chain, see $1INCH and $GEL token address which are same across multiple chains)

3) Now, Alice created a vault with 11000e18 TokenB;

4) Bob calls either of the Withdraw() function in vault and then withdraw() to receive 10000e18 TokenB.

5) In summary, one of the traps set by the attacker i.e Bob was activated by the deployment of TokenB and Alice was the victim. As a result, 10000e18 TokenB was stolen by the attacker i.e Bob.

Such issue can be prevented during the deployment of vault itself, i.e by checking the passed assets/tokens have code or not. This can be simply done by using openzeppelin's safeERC20.sol which takes care of token code existence check by default.

[0xRizwan]

On top of above issue, solmate's safeTransferLib.sol transfer functions are violating the solidity strict warnings with the use of low level call functions. solidity warns,

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

code existence must be checked especially for low level functions like call,staticcall and delegatecall.

Now, see the implementation of solmate safeTransferFrom:

    function safeTransferFrom(
        ERC20 token,
        address from,
        address to,
        uint256 amount
    ) internal {
        bool success;

        /// @solidity memory-safe-assembly
        assembly {
            // Get a pointer to some free memory.
            let freeMemoryPointer := mload(0x40)

            // Write the abi-encoded calldata into memory, beginning with the function selector.
            mstore(freeMemoryPointer, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
            mstore(add(freeMemoryPointer, 4), and(from, 0xffffffffffffffffffffffffffffffffffffffff)) // Append and mask the "from" argument.
            mstore(add(freeMemoryPointer, 36), and(to, 0xffffffffffffffffffffffffffffffffffffffff)) // Append and mask the "to" argument.
            mstore(add(freeMemoryPointer, 68), amount) // Append the "amount" argument. Masking not required as it's a full 32 byte type.

            success := and(
                // Set success to whether the call reverted, if not we check it either
                // returned exactly 1 (can't just be non-zero data), or had no return data.
                or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
                // We use 100 because the length of our calldata totals up like so: 4 + 32 * 3.
                // We use 0 and 32 to copy up to 32 bytes of return data into the scratch space.
                // Counterintuitively, this call must be positioned second to the or() call in the
                // surrounding and() call or else returndatasize() will be zero during the computation.
                call(gas(), token, 0, freeMemoryPointer, 100, 0, 32)
            )
        }

        require(success, "TRANSFER_FROM_FAILED");
    }

see,

                call(gas(), token, 0, freeMemoryPointer, 100, 0, 32)

This uses call function which is low level function and the current implementation is violating the solidity use of call since it does not check token code existence.

To be inline with solidity guidlines, the above function should check the token code existence by using below code,

function isContract(address addr) returns (bool) {
  uint size;
  assembly { size := extcodesize(addr) }
  return size > 0;
}

This was just for your information. You dont have to work on above code as openzeppelin's safeERC20.sol has already taken care of it.

I hope, this much information is enough for the validation of this issue. In addition, I have added 3 references above from sherlock and code4rena. However, if you need still clarifications, would be happy to clarify.

Thanks.

[reednaa]

I am sorry but I still do not understand how you get past these lines:

https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultVolatile.sol#L109-L113

From my understanding, ERC20(tokenAddress).balanceOf(address(this)); will either fail or return 0 when called which will revert.

[reednaa]

Based on on Discord conversation, we have decided to give you the relevant gas optimisations that this submission rasied. That is, convert Solmate into Solady. Because there is a significant amount of work related to integrating Solady over Solmate, I decided to do it on the working branch. As a result, you are still getting the majority of the gain but not all. You can find the change here: https://github.com/catalystdao/catalyst/commit/1972c8d266a3caf84737514df72aebb8ffb815b8

You can find the relevant results on the sheets.