Possible to use non-existing address for tokens

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0xc97a42dc381fd37f8049aa8f6a012b7d1dac46bd08d3517355d661f69158fb31 Severity: medium

Description:

Impact

Unexpected functionality: no transfers while transfer should happen, deploying vault with non-existing tokens. Note: will examine impact more thoroughly after the submission

Description

The Catalyst codebase uses Solmate's SafeTransferLib for transferring ERC20 tokens across the catalystdao/catalyst codebase. However the codebase does not check if the provided token addresses do really exist or not. The difference between openzeppelin's SafeERC20 and Solmate's SafeTransferLib is: while Openzeppelin checks if the token is a contract, Solmate does not.

solmate/src/utils/SafeTransferLib.sol

@dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller

Instances

File: CatalystChainInterface.sol

788:         ERC20(toAsset).safeTransferFrom(

804:         ERC20(toAsset).safeTransfer(toAccount, purchasedTokens);

888:             ERC20(toAsset).safeTransfer(msg.sender, expireShare);

891:             ERC20(toAsset).safeTransfer(targetVault, vaultShare);

944:         ERC20(toAsset).safeTransfer(refundTo, refundAmount);

File: CatalystFactory.sol

88:             ERC20(assets[it]).safeTransferFrom(

File: CatalystVaultAmplified.sol

381:                 ERC20(token).safeTransferFrom(

605:             ERC20(token).safeTransfer(msg.sender, weightedTokenAmount);

770:             ERC20(tokenIndexed[it]).safeTransfer(msg.sender, tokenAmount);

825:         ERC20(fromAsset).safeTransferFrom(msg.sender, address(this), amount);

826:         ERC20(toAsset).safeTransfer(msg.sender, out);

901:         ERC20(fromAsset).safeTransferFrom(msg.sender, address(this), amount);

1098:         ERC20(toAsset).safeTransfer(toAccount, purchasedTokens);

File: CatalystVaultCommon.sol

384:             ERC20(asset).safeTransfer(governanceFeeDestination(), governanceFeeAmount);

530:         ERC20(escrowToken).safeTransfer(fallbackAddress, escrowAmount);  // Would fail if there is no balance. To protect against this, the escrow amount should be removed from what can be claimed by users.

690:         ERC20(escrowToken).safeTransfer(refundTo, escrowAmount);

File: CatalystVaultVolatile.sol

379:             ERC20(token).safeTransferFrom(

458:             ERC20(token).safeTransfer(msg.sender, tokenAmount);

542:             ERC20(token).safeTransfer(msg.sender, tokenAmount);

583:         ERC20(fromAsset).safeTransferFrom(msg.sender, address(this), amount);

584:         ERC20(toAsset).safeTransfer(msg.sender, out);

645:         ERC20(fromAsset).safeTransferFrom(msg.sender, address(this), amount);

819:         ERC20(toAsset).safeTransfer(toAccount, purchasedTokens);

File: router/libraries/Payments.sol

33:             ERC20(token).safeTransfer(recipient, value);

52:             ERC20(token).safeTransfer(recipient, amount);

69:             if (balance > 0) ERC20(token).safeTransfer(recipient, balance);

107:         ERC20(token).safeTransferFrom(from, to, amount);

Recommendation

Consider using either OpenZeppelin's SafeERC20 instead of SafeTransferLib or adding additional checks to verify that indeed the provided token address is a valid contract.

hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4