Lack of check if token is a contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L94 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L127 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L167 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L228 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L231 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L365

Vulnerability details

Impact

solmate won't check if the token is a contract or not.

It's possible for an attacker to the create a vault for a non existing token with a deterministic address. The problem occurs if the token gets deployed later, and another user tries to use this token, the original attacker can withdraw the vault and steal user funds.

This type of attack (malicious interactions with non existing tokens) was used for the Qubit Finance hack. Check rekt and halborn reports.

Proof of Concept

The contracts are using transfer and safeTransfer from solmate. Solmate will not check for contract existance as described on their comment.

import {ERC20} from "@solmate/tokens/ERC20.sol";
import {SafeTransferLib} from "@solmate/utils/SafeTransferLib.sol";

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L4-L5

asset.safeTransferFrom(msg.sender, address(this), assets);

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L94

asset.safeTransfer(receiver, assets);

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L127

asset.transferFrom(msg.sender, address(this), shares);

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L167

asset.transfer(treasury, feeValue);

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L228

asset.transfer(receiver, entitledShares);

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L231

asset.transfer(_counterparty, idFinalTVL[id]);

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L365

Tools Used

Manual review.

Recommended Mitigation Steps

OpenZeppelin checks the code for the token address.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L36-L42

Pseudocode

address(token).code.length > 0

Consider using OpenZeppelin or checking for contract existance manually.

[MiguelBits]

looks like it is working as intended. we want to allow the deposits to contracts. I dont see the issue here

[HickupHH3]

token to be used is WETH for vault. the case for SemiFungibleVault can be argued since there could be subsequent implementations of other vaults to justify having this check, but given the current implementation, it's not something that needs to be considered.