Anyone can steal funds in the Contract Deployer

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L318

Vulnerability details

Impact

If ContractDeployer.sol ever holds funds, it could potentially be drained via the chained creation of new contracts.

Proof of Concept

When creating a contract the create/create2 functions will be called inside the contract deployer here: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L128-L150, which after doing validation on the codehash and created account, will call the _constructContract method here: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L308-L328.

This call sets the msg.value, and calls the L2EthToken contract to transfer funds from the bootloader to the constructed contract, then sets the msg.value for the next call. It will then use mimicCall to call the contract's constructor. However, if the created contract's constructor creates another contract, passing the msg.value to the new contract, the same create/create2 function will be invoked, and the _constructContract function will transfer ether from itself to the new contract, setting the msg.value again. An attacker can repeat this process and drain the ContractDeployer of any funds, since every time a new contract is created ether is transferred to the new contract from the ContractDeployer and the system will set the same msg.value to propogate it. This is similar to a reentrancy attack but a new contract being created is valid and will bypass the checks in the _nonSystemDeployOnAddress() function here: https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/ContractDeployer.sol#L249-L267

Tools Used

Manual Review

Recommended Mitigation Steps

Transfer from msg.sender instead of the ContractDeployer contract.

[miladpiri]

When the first contract creates the second contract, it is sending value to deployerContract. This value (not the old balance of deployerContract) will be used to be transferred from deployerContract to the second contract.

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[GalloDaSballo]

Agree with the sponsor, missing proof

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof