code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L550-L587

Vulnerability details

Impact

In the liquidate function from the Vault contract, the input argument _amountOut is used as if it was representing a value of asset amount and share amount at the same time which is impossible a there a conversion rate between them, this error will make liquidate function behave in an expected manner, not the one that was intended.

Proof of Concept

The issue is occurring in the liquidate function below :

function liquidate(
    address _account,
    address _tokenIn,
    uint256 _amountIn,
    address _tokenOut,
    uint256 _amountOut
) public virtual override returns (bool) {
    _requireVaultCollateralized();
    if (msg.sender != address(_liquidationPair))
      revert LiquidationCallerNotLP(msg.sender, address(_liquidationPair));
    if (_tokenIn != address(_prizePool.prizeToken()))
      revert LiquidationTokenInNotPrizeToken(_tokenIn, address(_prizePool.prizeToken()));
    if (_tokenOut != address(this))
      revert LiquidationTokenOutNotVaultShare(_tokenOut, address(this));
    if (_amountOut == 0) revert LiquidationAmountOutZero();

    uint256 _liquidableYield = _liquidatableBalanceOf(_tokenOut);

    // @audit _amountOut compared with _liquidableYield which represents an asset amount
    // @audit _amountOut is considered as an asset amount
    if (_amountOut > _liquidableYield)
      revert LiquidationAmountOutGTYield(_amountOut, _liquidableYield);

    _prizePool.contributePrizeTokens(address(this), _amountIn);

    if (_yieldFeePercentage != 0) {
        // @audit _amountOut used to increase fee shares so considered as representing a share amount
        _increaseYieldFeeBalance(
            (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut
        );
    }

    uint256 _vaultAssets = IERC20(asset()).balanceOf(address(this));

    // @audit _amountOut compared with _vaultAssets which represents an asset amount
    // @audit _amountOut is considered as an asset amount
    if (_vaultAssets != 0 && _amountOut >= _vaultAssets) {
      _yieldVault.deposit(_vaultAssets, address(this));
    }

    // @audit _amountOut representing a share amount minted to the _account
    _mint(_account, _amountOut);

    return true;
}

As you can see from the code above, the value of the argument _amountOut is used multiple times in the function logic and each time it is representing either an asset amount or a share amount which is impossible as there a conversion formula used to transform asset amount into share amount (and inversely) with the function _convertToShares (or _convertToAssets).

From the function comments i couldn't figure out what the value of _amountOut actually represents, but because there is also another argument given to the liquidate function which is _tokenOut == address(this), I'm supposing that _amountOut is representing a share amount which will mean that all the instances highlighted in the code above when _amountOut is considered as an asset amount are wrong.

Instance 1 :

// @audit _amountOut compared with _liquidableYield which represents an asset amount
if (_amountOut > _liquidableYield)
  revert LiquidationAmountOutGTYield(_amountOut, _liquidableYield);

Instance 2 :

// @audit _amountOut compared with _vaultAssets which represents an asset amount
if (_vaultAssets != 0 && _amountOut >= _vaultAssets) {
  _yieldVault.deposit(_vaultAssets, address(this));
}

And before comparing _amountOut to the asset amount values : _vaultAssets and _liquidableYield, its value should be converted to an asset amount with the function _convertToAssets.

This issue will cause problems for the protocol working as the liquidate function logic will not behave as expected (because it's comparing values that represents different things).

** Note : if _amountOut is actually representing an asset amount (not a share amount as i supposed), the issue is still valid because _amountOut is also used as being a share amount inside the liquidate function, in that case it should first be converted to a share amount with _convertToShares in order to get the correct behavior of the liquidate function.

Tools Used

Manual review

Recommended Mitigation Steps

To solve this issue i recommend to first convert the value of _amountOut in the liquidate function to an asset amount and store it in a local variable _amountOutToAsset, and in the function logic use the correct variable (either _amountOut or _amountOutToAsset) when interacting with a share amount or an asset amount.