Contracts allow sending ETH on calls which does not expect it

[code423n4]

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

Take for example the depositCollateral function. It's payable but the pool may not use ETH as collateral

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/Pool/Pool.sol#L175-L179

In the case where the user is performing a direct deposit then we'll pull in the erc20 collateral asset using the transferTokens function however there's no check for msg.value == 0 when transferring an ERC20 asset.

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/SavingsAccount/SavingsAccountUtil.sol#L98-L127

Similarly when depositing ETH from a savings account any ETH in msg.value will be lost.

Recommended Mitigation Steps

Add a check that msg.value is zero on all code paths which do not handle ETH.

Consider whether to simplify to just use WETH.

[ritik99]

We'll add extra checks on our end, but the underlying issue would stem from the end-user not performing proper checks - sending ETH during the ERC20 pulls when it isn't required. payable is necessary because there are instances where ETH could be the collateral

[0xean]

going to leave as low risk, certainly would be a user error, but doesn't mean it doesn't result in a bad outcome that could be mitigated with the checks.