audit: Proxy Deployment Vulnerable to Frontrunning

[Orland0x]

In the current implementation of the ProxyFactory contract, the deployment of a new proxy contract could be potentially frontrun. The frontrunner could deploy a proxy at the same predicted address but with different parameters. This issue arises due to the deployProxy method's reliance on salt for proxy address prediction and the absence of the msg.sender or initializer payload in the address computation parameters. One mitigation strategy would be to include the msg.sender or the initializer payload in the salt. This change could make the address prediction unique for every user and initializer, thus preventing potential frontrun attacks. Another mitigation is to properly check that the transaction was succesful when deployProxy() is called, as the transaction will revert if a proxy has already been deployed at the same address.

[Orland0x]

Fix

Compute CREATE2 salt as: bytes32 salt = keccak256(abi.encode(msg.sender, keccak256(initializer), saltNonce));

where saltNonce is passed to deployProxy() as an argument.

[pscott]

Out of curiosity, why go for this instead of checking whether deployProxy was succesful or not?

[Orland0x]

Out of curiosity, why go for this instead of checking whether deployProxy was succesful or not?

A spammer could still mess things up by front running deployments, they could repeatedly do it and prevent deploying a space.

by putting the msg.sender (ie the mana account) in the salt computation, its impossible to frontrun

[pscott]

true