Re-org attack in VaultFactory contract (function deployVault)

[code423n4]

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

[c4-judge]

Picodes marked the issue as duplicate of #416

[c4-judge]

Picodes marked the issue as satisfactory