Sturdy contest - TerrierLover's results

Lines of code

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

Vulnerability details

Impact

It is possible that users cannot receive ETH if sending ETH to users is not successful because the require check is unreachable.

Proof of Concept

_withdrawFromYieldPool function executes the following code if user requests ETH withdrawal.

if (_asset == address(0)) {
    // Case of ETH withdraw request from user, so exchange stETH -> ETH via curve
    uint256 receivedETHAmount = CurveswapAdapter.swapExactTokensForTokens(
    _addressesProvider,
    _addressesProvider.getAddress('STETH_ETH_POOL'),
    LIDO,
    ETH,
    _amount,
    200
    );

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

Since the require check is an unreachable code, users cannot receive ETH if sending ETH to users is not successful.

Tools Used

static code analysis

Recommended Mitigation Steps

Changing the order of execution like this can prevent this issue.

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

Here are QA reports per file.

CollateralAdapter.sol

[QA-1] address(0) checks are mising at addCollateralAsset function

addCollateralAsset function does not have any address(0) check.

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/CollateralAdapter.sol#L43-L46

function addCollateralAsset(
    address _externalAsset,
    address _internalAsset,
    address _acceptVault
) external onlyAdmin {
    _assetToVaults[_externalAsset] = _acceptVault;
    _collateralAssets[_externalAsset] = _internalAsset;
}

addCollateralAsset function is callable only by admin, but setting proper address(0) checks would be useful.

LidoVault.sol

[QA-2] withdrawOnLiquidation function is missing

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

LidoVault.sol does not override the withdrawOnLiquidation function. If not having this functionality on LidoVault is expected, LidoVault.sol can implement the withdrawOnLiquidation function which only has revert() or other appropriate logic to specify that LidoVault does not have this functionality.