Sturdy contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L125

Vulnerability details

Impact

When withdrawing ETH collateral from LidoVault with withdrawCollateral(), stETH is exchanged to ETH via Curve while using a slippage value of 2% (200, L136).

The resulting exchanged amount receivedETHAmount is then validated to be larger than the requested withdrawal _amount minus 1% slippage:

require(withdrawAmount >= _amount.percentMul(99_00), Errors.VT_WITHDRAW_AMOUNT_MISMATCH);

If the curve stETH pool is unbalanced and therefore causes a slippage of ~2%, the GeneralVault.withdrawCollateral call will revert.

Proof of Concept

GeneralVault.sol#L125

Tools Used

Manual review

Recommended mitigation steps

Make sure to use the same value for slippage checks within GeneralVault and LidoVault.

Lines of code

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

Vulnerability details

Impact

The LidoVault contract uses a low level Solidity .call() to send ETH to the user, but the require to check the sent return value is placed after the return, hence the sent success return value is not checked. This could cause a silent fail transferring ETH and loss of funds for the user.

Proof of Concept

LidoVault.sol#L141-L142

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

Tools Used

Manual review

Recommended mitigation steps

Move the require check before the return.

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L131

Vulnerability details

Impact

If a borrower deposits Curve LP tokens into the ConvexCurveLPVault contract via the payable function GeneralVault.depositCollateral(address _asset, uint256 _amount) and accidentally sends a non-zero Ether value with it, then the Ether value sent will be locked within the contract without the ability to retrieve it.

Proof of Concept

ConvexCurveLPVault._depositToYieldPool

Tools Used

Manual review

Recommended mitigation steps

Make sure the function ConvexCurveLPVault._depositToYieldPool reverts if msg.value > 0.

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L121-L124

Vulnerability details

Impact

Withdrawing ETH collateral via the withdrawCollateral function using type(uint256).max for the _amount parameter reverts the transaction due to _asset being the zero-address and IERC20Detailed(_asset).decimals() not working for native ETH.

Proof of Concept

GeneralVault.sol#L121-L124

if (_amount == type(uint256).max) {
    uint256 decimal = IERC20Detailed(_asset).decimals(); // @audit-info does not work for native ETH. Transaction reverts
    _amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**decimal);
}

Tools Used

Manual review

Recommended mitigation steps

Check _asset and use hard coded decimal value (18) for native ETH

QA Report

Table of Contents

• Low Risk
  • [L-01]: Events not emitted for important state changes
  • [L-02]: Zero-address checks are missing
  • [L-03]: Restrict ETH funds receivable by LidoVault to be only from Curve exchange

Low Risk

[L-01]: Events not emitted for important state changes

Description

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

YieldManager.setExchangeToken()
YieldManager.registerAsset()
YieldManager.setCurvePool()
ConvexCurveLPVault.setConfiguration()
CollateralAdapter.addCollateralAsset()

Recommended mitigation steps

Emit events for state variable changes.

[L-02]: Zero-address checks are missing

Description

Zero-address checks are a best-practice for input validation of critical address parameters. While the codebase applies this to most most cases, there are many places where this is missing in constructors and setters.

Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens or force redeployment of contracts.

Findings

YieldManager.sol#L74

_assetsList[_assetsCount] = _asset;

ConvexCurveLPVault.sol#L41

curveLPToken = _lpToken;

Recommended mitigation steps

Add zero-address checks, e.g.:

require(_asset != address(0), "Zero-address");

[L-03]: Restrict ETH funds receivable by LidoVault to be only from Curve exchange

Description

Native ETH fund transfers into the LidoVault contract are only expected from the curveExchange contract. Hence, it would be good to restrict incoming ETH fund transfers to prevent accidental native fund transfers from other sources.

Findings

LidoVault.sol#L24

/**
  * @dev Receive Ether
  */
receive() external payable {}

Recommended mitigation steps

Modify the receive() function to only accept transfers from the wrapped token contract:

receive() external payable {
  require(msg.sender == address(curveExchange), 'Only curve exchange');
}