code423n4 commented 1 year ago

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 clone contract using create, where getting the address depends only on the VaultFactory nonce.

Reorganization can occur in all EVM chains. In Ethereum, where pooltogether is currently deployed, this is not "super common" but still happening, being the last one less than a year ago: https://decrypt.co/101390/ethereum-beacon-chain-blockchain-reorg

An issue can arise when users rely on getting an address beforehand, or try to deploy a clone of a position with the same address on different EVM chains, any funds sent to the new clone could potentially be withdrawn by someone else. In general, this can lead to the theft of users' funds.

An example of a similar error: https://code4rena.com/reports/2023-01-rabbithole#m-01-questfactory-is-suspicious-of-the-reorg-attack

Proof of Concept

Imagine that Alice deploys a clone of a position and then sends funds to it. Bob sees that the network block is being reorganized and calls deployVault. Thus, a clone of the position is created with the address to which Alice sends funds. Alice's transactions are then executed and Alice transfers the funds to Bob's position contract.

Tools Used

Manual review

Recommended Mitigation Steps

Deploy Vault via create2 with salt which includes msg.sender and _asset address.

Assessed type

Context

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #416