Improper handling of cases when withdrawable assets = 0

[code423n4]

Lines of code

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

Vulnerability details

Impact

• Improper handling of cases when withdrawable assets = 0
• The vault will not function correctly.

Proof of Concept

The function _currentExchangeRate and _isVaultCollateralized of Vault are implemented as follows:

function _currentExchangeRate() internal view returns (uint256) {
    uint256 _totalSupplyAmount = _totalSupply();
    uint256 _totalSupplyToAssets = _convertToAssets(
      _totalSupplyAmount,
      _lastRecordedExchangeRate,
      Math.Rounding.Down
    );

    uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this));

    if (_withdrawableAssets > _totalSupplyToAssets) {
      _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
    }

    if (_totalSupplyAmount != 0 && _withdrawableAssets != 0) {
      return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);
    }

    return _assetUnit;
  }

function _isVaultCollateralized() internal view returns (bool) {
    return _currentExchangeRate() >= _assetUnit;
}

The function Calculate exchange rate between the amount of assets withdrawable from the YieldVault and the amount of shares minted by this Vault.
However, if _withdrawableAssets is 0, then the function returns _assetUnit (which means 1-1 ratio). This means that even when the vault has no withdrawable assets from _yieldVault, it's still considered collateralized.

To illustrate the oddity of this special case, consider when _withdrawableAssets = 1 and _totalSupplyAmount > 0; in this scenario, _currentExchangeRate returns 0 and the vault is considered under-collateralized (since 1 < _assetUnit). However, if _withdrawableAssets = 0, the vault is considered collateralized.

This case has profound impact since a lot of vault logic is based on the vault's collateralized status.

Below is a POC for the above example, for ease of testing, let's place these 2 test cases
in file vault/test/unit/Vault/Deposit.t.sol under contract VaultDepositTest,
then test them using commands:
forge test --match-path test/unit/Vault/Deposit.t.sol --match-test testOneWithdrawableAmount -vvvv
forge test --match-path test/unit/Vault/Deposit.t.sol --match-test testZeroWithdrawableAmount -vvvv

testOneWithdrawableAmount is to demonstrate when _withdrawableAssets = 1 and the vault is considered not collateralized, testZeroWithdrawableAmount is to demonstrate when _withdrawableAssets = 0 and the vault is considered collateralized.

function testZeroWithdrawableAmount() public {
    vm.startPrank(alice);
    uint256 _amount = 1000e18;
    underlyingAsset.mint(alice, _amount);
    underlyingAsset.approve(address(vault), type(uint256).max);

    vault.deposit(_amount, alice);
    

    // Now make the withdrawable asset = 0
    // Burn the balance of yieldVault
    uint256 yieldVaultAsset = underlyingAsset.balanceOf(address(yieldVault));
    underlyingAsset.burn(address(yieldVault), yieldVaultAsset);

    assertEq(underlyingAsset.balanceOf(address(yieldVault)), 0);

    // Although the vault has no asset withdrawable in yieldVault
    // the exchange rate is 10**18 = assetUnit and the vault is "collateralized"
    assertEq(yieldVault.maxWithdraw(address(vault)), 0);
    assertEq(vault.exchangeRate(), 10**18);
    assertEq(vault.isVaultCollateralized(), true);
  }

  function testOneWithdrawableAmount() public {
    vm.startPrank(alice);
    uint256 _amount = 1000e18;
    underlyingAsset.mint(alice, _amount);
    underlyingAsset.approve(address(vault), type(uint256).max);

    vault.deposit(_amount, alice);
    

    // Now make the withdrawable asset = 0
    // Burn the balance of yieldVault
    uint256 yieldVaultAsset = underlyingAsset.balanceOf(address(yieldVault));
    underlyingAsset.burn(address(yieldVault), yieldVaultAsset -1);

    assertEq(underlyingAsset.balanceOf(address(yieldVault)), 1);

    // vault only has 1 asset token withdrawable, and the exchangeRate is 0
    // the vault is not collateralized
    assertEq(yieldVault.maxWithdraw(address(vault)), 1);
    assertEq(vault.exchangeRate(), 0);
    assertEq(vault.isVaultCollateralized(), false);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Since _withdrawableAssets is the dividend, there seems to be no harm in removing the check if _withdrawableAssets = 0. Therefore, I recommend removing it from the condition.

Assessed type

Invalid Validation

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[Picodes]

There is definitely a bug here, but I don't see why it should be high severity. "This case has a profound impact since a lot of vault logic is based on the vault's collateralized status" is a bit light to pass the burden or proof. At first sight, as they are no funds in the vault anymore there is no loss of funds. Then there is the case where assets are readded to the vault for some reason, but this would be a Medium severity scenario considering it's very unlikely and not described by the report.

[c4-judge]

Picodes marked the issue as satisfactory

[ktg9]

Hi @Picodes, thank you for your response!

Sorry I didn't make it clearer. As you can see in the report, the affected function is _currentExchangeRate, so not only the Vault incorrectly specified as collateralized, the currentExchangeRate also = 1 (assetUnit). These 2 functions affects many other functions, for example:

• Function mintYieldFee requires that vault is collateralized, so user can mint yield fee even when it's not
• Function liquidate requires that vault is collateralized, so user can liquidate asset even when vault's withdrawable asset = 0
• For deposit function, first deposit will calculate maxDeposit to limit the input tokens:

  function maxDeposit(address) public view virtual override returns (uint256) {
    return _isVaultCollateralized() ? type(uint96).max : 0;
  }

if the Vault is under-collateralized, user can't deposit since maxDeposit will return 0; however, user can still deposit in this case

• User calls exchangeRate and receives 1, this can lead them to bad decisions.

To conclude, I agree that this will not lead to direct loss of funds for the vault, but will greatly impact user, making them send tokens to the vault (through liquidate or deposit) when it's not collateralized and will potentially suffer economically. I think it should be marked as High severity.

[Picodes]

@ktg9 I still think that the original reports perfectly fit with the definition of Med severity: "leak value with a hypothetical attack path with stated assumptions, but external requirements". The described scenarios require that we are in a situation with withdrawable assets = 0 and that some users behave incorrectly. Also note that I can't consider scenarios that are not described in the original report.

[PierrickGT]

This issue has been fixed and the exchange rate between the Vault and YieldVault is now 1 to 1.
We've replaced the exchange rate function by a collateral one, which simplifies the conversions from shares to assets and assets to shares.

If the Vault is collateralized, meaning the amount of withdrawable assets from the YieldVault is greater than the amount of underlying assets supplied to the YieldVault, then users can only withdraw the amount they deposited.
Otherwise, any remaining collateral within the YieldVault is available and distributed proportionally among depositors.

https://github.com/GenerationSoftware/pt-v5-vault/blob/8985bead1be85ae6822cd329933f5a53de05c237/src/Vault.sol#L1208

[asselstine]

Fixed in GenerationSoftware/pt-v5-vault#18