search

code-423n4 / 2023-04-caviar-findings

9 stars 4 forks source link

The Factory logic of `predictPoolDeploymentAddress()` may be broken if the `privatePoolImplementation` is changed #956

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L168

Vulnerability details

Impact

A new pool is created using the Factory.create() function, which takes a salt parameter to calculate the address of the new pool:

// deploy a minimal proxy clone of the private pool implementation
privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L92

It can be assumed that the public function predictPoolDeploymentAddress(salt) can be used by other developers and integrators to quickly calculate the addresses of previously deployed pools.

However, the Factory administrator can change the address of the pool implementation using the setPrivatePoolImplementation() function. In this case, the function predictPoolDeploymentAddress(salt) will return a new address for an old salt:

function predictPoolDeploymentAddress(bytes32 salt) public view returns (address predictedAddress) {
    predictedAddress = privatePoolImplementation.predictDeterministicAddress(salt, address(this));
}

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L165-L170

This means that if a developer or integrator creates a pool with their own salt and then looks for their pool using the public predictPoolDeploymentAddress(salt) method, then in the future, when the administrator changes the privatePoolImplementation, a different address will be returned for the same salt.

This could allow an attacker to deploy their own pool with unexpected balances and attack the protocols or bots that mistakenly believed that the pool address for a particular salt would remain unchanged.

Recommended Mitigation Steps

It is recommended to either make the predictPoolDeploymentAddress() function private so that other developers are not tempted to use it, or to add the privatePoolImplementation parameter to the function so that the developer can obtain the exact address of previously deployed pools.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

outdoteth marked the issue as sponsor confirmed

c4-sponsor commented 1 year ago

outdoteth marked the issue as sponsor acknowledged

c4-sponsor commented 1 year ago

outdoteth marked the issue as disagree with severity

GalloDaSballo commented 1 year ago

If the implementaton is changed, older pools will not be trackable, but new pools will be predictable

I also believe the logic can be easily re-implemented by integrators

The newly deployed pool address is also returned after creating: https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L84

For those reasons, also considering that the function is view and is not used anywhere else am downgrading to QA Low

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-b