code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/VaultController.sol#L646-L697

Vulnerability details

Impact

The liquidateVault function can be frontrunned every time by any bot frontrunner watching to this function in the mempool.

Proof of Concept

function liquidateVault(
    uint96 _id,
    address _assetAddress,
    uint256 _tokensToLiquidate
  ) external override paysInterest whenNotPaused returns (uint256 _toLiquidate) {
    // cannot liquidate 0
    if (_tokensToLiquidate == 0) revert VaultController_LiquidateZeroTokens();
    // check for registered asset
    if (tokenAddressCollateralInfo[_assetAddress].tokenId == 0) revert VaultController_TokenNotRegistered();

    // calculate the amount to liquidate and the 'bad fill price' using liquidationMath
    // see _liquidationMath for more detailed explaination of the math
    (uint256 _tokenAmount, uint256 _badFillPrice) = _liquidationMath(_id, _assetAddress, _tokensToLiquidate);
    // set _tokensToLiquidate to this calculated amount if the function does not fail
    if (_tokenAmount > 0) _tokensToLiquidate = _tokenAmount;
    // the USDA to repurchase is equal to the bad fill price multiplied by the amount of tokens to liquidate
    uint256 _usdaToRepurchase = _truncate(_badFillPrice * _tokensToLiquidate);
    // get the vault that the liquidator wishes to liquidate
    IVault _vault = _getVault(_id);

    // decrease the vault's liability
    _vault.modifyLiability(false, (_usdaToRepurchase * 1e18) / interest.factor);

    // decrease the total base liability
    totalBaseLiability -= _safeu192((_usdaToRepurchase * 1e18) / interest.factor);

    // decrease liquidator's USDA balance
    usda.vaultControllerBurn(_msgSender(), _usdaToRepurchase);

    // withdraw from convex
    CollateralInfo memory _assetInfo = tokenAddressCollateralInfo[_assetAddress];
    if (_vault.isTokenStaked(_assetAddress)) {
      _vault.controllerWithdrawAndUnwrap(_assetInfo.crvRewardsContract, _tokensToLiquidate);
    }

    uint192 _liquidationFee = getLiquidationFee(uint192(_tokensToLiquidate), _assetAddress);

    // finally, deliver tokens to liquidator
    _vault.controllerTransfer(_assetAddress, _msgSender(), _tokensToLiquidate - _liquidationFee);
    // and the fee to the treasury
    _vault.controllerTransfer(_assetAddress, owner(), _liquidationFee);
    // and reduces total
    _modifyTotalDeposited(_tokensToLiquidate, _assetAddress, false);

    // this mainly prevents reentrancy
    if (_getVaultBorrowingPower(_vault) > _vaultLiability(_id)) revert VaultController_OverLiquidation();

    // emit the event
    emit Liquidate(_id, _assetAddress, _usdaToRepurchase, _tokensToLiquidate - _liquidationFee, _liquidationFee);
    // return the amount of tokens liquidated (including fee)
    _toLiquidate = _tokensToLiquidate;
  }

Since the function basically only cares if the vault is underwater every time anyone would see the opportunity to liquidate a vault they would get front-runned by a bot watching this function. It would heavily discourage people from finding these under the water vaults to gain some rewards since the reward would go to the bot frontrunner.