Sturdy contest - StErMi's results

Judge has assessed an item in Issue #103 as High risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L79-L104 https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/ConvexCurveLPVault.sol#L131-L149

Vulnerability details

Impact

LidoVault and ConvexCurveLPVault both inherit from GeneralVault that implement the method depositCollateral. This method has the keyword payable so it allows users to send ETH with the tx. _depositToYieldPool that is called inside depositCollateral in both LidoVault and ConvexCurveLPVault are not preventing the user to transfer ETH to the vault when they are depositing only token. Those ETH funds of the user would be stuck forever in the Vault contract because there's no way for the contract owner to sweep and send them to a customer receiver

Proof of Concept

Here's a test to simulate it. The test has been run on the complete code that can be found here https://github.com/sturdyfi/code4rena-may-2022

To run it FORK=main SKIP_DEPLOY=true TS_NODE_TRANSPILE_ONLY=1 hardhat test ./test-suites/test-local/*.spec.ts --network localhost

And I've created the test inside a /test-suites/test-local/ folder

import { expect } from 'chai';
import { makeSuite, SignerWithAddress, TestEnv } from '../test-sturdy/helpers/make-suite';
import { BigNumberish, ethers } from 'ethers';
import { DRE, impersonateAccountsHardhat } from '../../helpers/misc-utils';
import { convertToCurrencyDecimals } from '../../helpers/contracts-helpers';
import { APPROVAL_AMOUNT_LENDING_POOL } from '../../helpers/constants';

const { parseEther } = ethers.utils;

const prepareCollateralForUser = async (
  testEnv: TestEnv,
  user: SignerWithAddress,
  amount: BigNumberish
) => {
  const { STECRV_LP } = testEnv;
  const ethers = (DRE as any).ethers;

  // block number: 14749000, balance: 737.243
  const LPOwnerAddress = '0x43378368d84d4ba00d1c8e97ec2e6016a82fc062';
  await impersonateAccountsHardhat([LPOwnerAddress]);
  const signer = await ethers.provider.getSigner(LPOwnerAddress);

  //transfer to borrower
  await STECRV_LP.connect(signer).transfer(user.address, amount);
};

makeSuite('ConvexSTETHVault', (testEnv: TestEnv) => {
  it('send yield to YieldManager', async () => {
    const { convexSTETHVault, deployer, STECRV_LP } = testEnv;
    const ethers = (DRE as any).ethers;

    // Prepare some STECRV_LP for depositor
    const assetAmountToDeposit = await convertToCurrencyDecimals(STECRV_LP.address, '700');
    await prepareCollateralForUser(testEnv, deployer, assetAmountToDeposit);

    // allow token transfer to this vault
    await STECRV_LP.connect(deployer.signer).approve(
      convexSTETHVault.address,
      assetAmountToDeposit
    );

    const beforePooledEther = await ethers.provider.getBalance(convexSTETHVault.address);

    // deposit collateral
    await convexSTETHVault
      .connect(deployer.signer)
      .depositCollateral(STECRV_LP.address, assetAmountToDeposit, { value: parseEther('1') });

    const currentPooledEther = await ethers.provider.getBalance(convexSTETHVault.address);

    // This sould be not true because user should not be able to deposit both curveLPToken and ETH at the same time
    expect(currentPooledEther.sub(beforePooledEther)).to.be.equal(parseEther('1'));
  });
});

makeSuite('LidoVault', (testEnv) => {
  it('User can deposit both LIDO and Ether at the same time. ETH are "lost".', async () => {
    const { users, lidoVault, lido } = testEnv;
    const ethers = (DRE as any).ethers;
    const borrower = users[1];

    const stETHOwnerAddress = '0x06920C9fC643De77B99cB7670A944AD31eaAA260';
    const depositStETH = '1';
    const amountStETHtoDeposit = await convertToCurrencyDecimals(lido.address, depositStETH);
    //Make some test stETH for borrower
    await impersonateAccountsHardhat([stETHOwnerAddress]);
    const signer = await ethers.provider.getSigner(stETHOwnerAddress);
    await lido.connect(signer).transfer(borrower.address, amountStETHtoDeposit);
    //approve protocol to access depositor wallet
    await lido.connect(borrower.signer).approve(lidoVault.address, APPROVAL_AMOUNT_LENDING_POOL);

    const beforePooledEther = await ethers.provider.getBalance(lidoVault.address);

    await lidoVault
      .connect(borrower.signer)
      .depositCollateral(lido.address, amountStETHtoDeposit, { value: parseEther('1') });

    const currentPooledEther = await ethers.provider.getBalance(lidoVault.address);

    // This sould be not true because user should not be able to deposit both Lido and ETH at the same time
    expect(currentPooledEther.sub(beforePooledEther)).to.be.equal(parseEther('1'));
  });
});

Tools Used

manual + hardhat test

Recommended Mitigation Steps

Here's an example of code that would revert when the user should not have sent ETH along with the deposit of the normal token.

LidoVault._depositToYieldPool

function _depositToYieldPool(address _asset, uint256 _amount) internal override returns (address, uint256) {
	// code ...
	if (_asset == address(0)) {
	  // code ...
	} else {
+	  // Check that the user has not send ETH togheter with LIDO
+	  require(msg.value == 0, "Only LIDO accepted (no ETH)");
	  

	  // code ...
	}

	// code ...
}

ConvexCurveLPVault._depositToYieldPool changes

function _depositToYieldPool(address _asset, uint256 _amount) internal override returns (address, uint256) {
+	  // Check that the user has not send ETH togheter with CurveLPToken
+	  require(msg.value == 0, "Only CurveLPToken accepted (no ETH)");

	// ... code
}

Judge has assessed an item in Issue #103 as Medium risk. The relevant finding follows:

CollateralAdapter

initialize function does not check input parameter

The initialize function should check that _provider is != address(0) otherwise the contract would be unusable forever given that there's no way to update it.

addCollateralAsset function does not check user parameter

The functions should do these checks

• _externalAsset != address(0)
• _internalAsset != address(0)
• _acceptVault != address(0)

addCollateralAsset miss event emission

This is a critical function that should emit an event when a vault and internal asset is associated to an external asset

addCollateralAsset allow overriding existing values

I don't know if this is the intention of the function but for me because of the name add it means that this should only allow to add new collateral records.

For this reason:

• the function should revert if you try to override an existing record for _assetToVaults and _collateralAssets. If _collateralAssets[_externalAsset] != address(0) it should revert
• a function updateCollateralAsset that allow the override should be created if there's a need. The function should emit an event.
• a function removeCollateralAsset that allow to remove a collateral asset should be created if needed. The function should emit an event.

Without checks on parameters value, override of existing collateral records and event emission the admin can swap information when needed

LendingPoolCollateralManager.liquidationCall is using both getAcceptableVault and getInternalCollateralAsset. Without the previous check and event emission an admin could manipulate those values, overriding them and swapping them back after a liquidationCall

YieldManager

initialize function does not check input parameter

The initialize function should check that _provider is != address(0) otherwise the contract would be unusable forever given that there's no way to update it.

setCurvePool is missing parameters validation

These checks should be made

• _tokenIn != address(0)
• _tokenOut != address(0)

setCurvePool is missing event emission

Setters should always emit event to track changes on external monitoring tools

registerAsset allow registering empty and duplicate assets

At the moment the function is not checking

• that _asset is != address(0)
• that the asset has been already registered

Because of empty address distributeYield could revert easily because you can register a normal token, then empty token, and then a normal token. As a result, you would be forced to call many times distributeYield to avoid those reverting registered assets

Having duplicates instead make the distributeField waste tons of gas for nothing because it would try to distribute the yield for the same asset multiple time. After the first asset distribution, all the other would just do nothing (no more yield) or revert (it depends on the implementation)

To prevent having duplicates, you could have an additional mapping to store _assetManaged(address => bool) and revert here if _assetManaged[_asset] == true

registerAsset is missing event emission

Setters should always emit event to track changes on external monitoring tools

GeneralVault

initialize function does not check input parameter

The initialize function should check that _provider is != address(0) otherwise the contract would be unusable forever given that there's no way to update it.

LidoVault

_withdrawFromYieldPool is returning before checking if receivedETHAmount has been received by _to

This is part of the _withdrawFromYieldPool code

// send ETH to user
(bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
// @audit - early return will make skip the requirement. In this case if the send to `_to` fail the funds will be stuck to the LidoVault asset
return receivedETHAmount;
require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

I've checked with a test on foundry and because there's an early return even if sent is false (so it should revert) the tx does not revert because there's an early return before the require. This means that for whichever reason (that I've not found tbh) the call revert, the user would not receive their ETH but also would not receive it anymore because stAsset have been already withdrawn from the user balance.

ConvexCurveLPVault

processYield could be prone to out-of-gas revert because of unbound loop

Because the unbound loop over extraRewardsLength the whole processYield could go out-of-gas and revert.

A possible solution would be to split processYield and create an additional function called processExtraYield where those extra yields would be managed. At worst only that function would fail and not the most important part where it tries to harvest the "main" yields.

processExtraYield should be declared inside GeneralVault as a virtual function if you want to follow the same code standard as processYield

ConvexCurveLPVault

_transferYield could return early if yieldAmount is 0 preventing to waste gas

_transferYield could return early if yieldAmount is 0 preventing to waste gas

LidoVault

processYield could return early if yieldStETH is 0 preventing to waste gas

processYield could return early if yieldStETH is 0 preventing to waste gas

YieldManager

distributeYield could be optimized

1. Inside the loop _convertAssetToExchangeToken could be called only if _amount > 0
2. Perform the block code identified by // 2. convert from exchange token to other stable assets via curve swap only if exchangedAmount > 0