Closed code423n4 closed 2 years ago
We are depositing the ETH in the receive function of NestedFactory
ETH -> WETH deposit is done on NestedFactory at L88-92:
receive() external payable {
if (msg.sender != address(withdrawer)) {
weth.deposit{ value: msg.value }();
}
}
Lines of code
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L169-L188
Vulnerability details
withdrawETH() effectively do not control the output token result of withdrawal as Vault token is ETH in this case, while WETH balance is controlled and no ETH -> WETH deposit is done. I.e. any calls to withdrawETH with non-zero minAmountOut will fail as WETH balance being controlled there does not change.
Setting severity to high as this is slippage control functionality unavailability (without conditions, as the described situation is permanent), which is a violation of system's core logic, and also it opens up the possibility for drainage of funds of big enough scale as that's one of the frequent operations.
Proof of Concept
withdrawETH() calls _withdrawAndRemoveLiquidity128() with ETH as target output token, but then passes WETH to getOutputAmounts():
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L169-L188
As no conversion is made the ETH delivered aren't effectively anyhow controlled, which is the main issue here.
Also, as WETH is always not delivered and any attempt to call the withdrawETH() with non-zero minAmountOut will be reverted (now tests don't seem to cover non-zero minAmountOut case).
Recommended Mitigation Steps
Similarly to depositETH issue, consider expanding balance check mechanics to ETH native case and using it in withdrawETH() to control for slippage in the output token terms: