depositAsset() transfers tokens from the sender without checking that the amount was indeed received. If the protocol were to accept other tokens than stETH, rETH, and cbETH, there is a possibility that some of the newly accepted tokens have custom ERC20 logic that implement fees. Since the amount of RSETH minted depends on how much was exactly deposited (uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);), this would pause a big accounting problem. In the worst case, protocol could lose funds as it mints more than what was deposited, and it will also fire a wrong event AssetDeposit since the depositAmount is not correct, throwing off off-chain logging services.
This is not a high-severity, as I am assuming the accepted ERC20-tokens are those present in the deployment script. This will become a problem if the protocol plan to expands to other ERC20-tokens.
Lines of code
https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L119
Vulnerability details
Impact
depositAsset() transfers tokens from the sender without checking that the amount was indeed received. If the protocol were to accept other tokens than stETH, rETH, and cbETH, there is a possibility that some of the newly accepted tokens have custom ERC20 logic that implement fees. Since the amount of RSETH minted depends on how much was exactly deposited (
uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
), this would pause a big accounting problem. In the worst case, protocol could lose funds as it mints more than what was deposited, and it will also fire a wrong eventAssetDeposit
since thedepositAmount
is not correct, throwing off off-chain logging services.This is not a high-severity, as I am assuming the accepted ERC20-tokens are those present in the deployment script. This will become a problem if the protocol plan to expands to other ERC20-tokens.
Proof of Concept
Tools Used
Manual review.
Recommended Mitigation Steps
The amounts received should be validated.
Assessed type
Invalid Validation