ERC20 transfer return value not checked

[code423n4]

Lines of code

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

Vulnerability details

Impact

According to EIP-20, transfer and transferFrom return a boolean success value. They can optionally revert when the tranfer does not succeed (which many tokens do), but this is no requirement. Vault does not check the return value of these calls. Because of that, it is possible to get free shares and steal assets from other users (when the withdrawal is performed).

Proof Of Concept

The vault is used with a token that does not revert when the transfer does not succeed. A user calls deposit without approving the smart contract. The asset transfer does not succeed, but this is not detected by the vault. Because of this, the user still receives the shares. He can then later call withdraw to get the corresponding amount for his shares, which in this case is taken from other users that deposited into this vault

Recommended Mitigation Steps

Use OpenZeppelins SafeTransfer library. This library also handles tokens correctly that do not return a value, which also exist.

[3xHarry]

dup #499