Sturdy contest - p4st13r4's results

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L142

Vulnerability details

Impact

Affected code: https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L142

In LidoVault.sol, _withdrawFromYieldPool() transfers the collateral back to the user, but it does not check that the transfer was successful:

// send ETH to user
(bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
return receivedETHAmount;
require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

Tools Used

Editor

Recommended Mitigation Steps

The return statement should be moved down, and the require(sent)should be called right after the .call().

Low Risk Issues

[L-01] In GeneralVault.sol, ETH address is misleading

In the GeneralVault contract there is a constant, ETH, set to a magic value: https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/GeneralVault.sol#L47

This is deceiving because no-one is actually using that constant, and in the concrete vaults that deal with eth (like the LIDO one), address(0) is used to represent user making deposits in eth: https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L86

Thus we suggest removing that constant because is useless and possibly deceiving for a reader

Non-critical Issues

[N-01] Use of saveApprove() deprecated function

The usage of OZ safeApprove() is deprecated as described in the latest OZ implementation: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bfff03c0d2a59bcd8e2ead1da9aed9edf0080d05/contracts/token/ERC20/utils/SafeERC20.sol#L38-L45

It’s currently used 3 times throughout the codebase, and it should be replaced with safeIncreaseAllowance() and safeDecreaseAllowance()