`VaultFactory.computeWithdrawVaultAddress()` and `VaultFactory.computeNodeELRewardVaultAddress()` return wrong addresses if `VaultFactory.vaultProxyImplementation` has been updated

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/factory/VaultFactory.sol#L62-L79 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/factory/VaultFactory.sol#L93-L97 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionlessPool.sol#L85-L120

Vulnerability details

Impact

Both the VaultFactory.computeWithdrawVaultAddress() and VaultFactory.computeNodeELRewardVaultAddress() functions use call ClonesUpgradeable.predictDeterministicAddress(vaultProxyImplementation, salt) to determine which address to return. This library function is guaranteed to return the same address as ClonesUpgradeable.cloneDeterministic(vaultProxyImplementation, salt) when given the same vaultProxyImplementation and salt values.

However, the value of vaultProxyImplementation may change via admin call to VaultFactory.updateVaultProxyAddress(), causing VaultFactory.computeWithdrawVaultAddress() and VaultFactory.computeNodeELRewardVaultAddress() to return wrong addresses, of non-existing contracts.

As a result, the PermissionlessPool.preDepositOnBeaconChain() function, which internally calls VaultFactory.computeWithdrawVaultAddress() would be misfunctioning yet won't revert - performing deposits with wrong withdrawCredential and locking out funds.

Recommended Mitigation Steps

Don't allow the admin to update VaultFactory.vaultProxyImplementation or keep track of previous values of VaultFactory.vaultProxyImplementation via a mapping that will map addresses returned by VaultFactory.deployWithdrawVault() and VaultFactory.deployNodeELRewardVault() to their corresponding value of VaultFactory.vaultProxyImplementation at the time of their deployment.

Assessed type

Other

[manoj9april]

Admin is a multi sig and before updating we will make sure we pause addValidatorKeys function

[c4-sponsor]

manoj9april marked the issue as disagree with severity

[c4-sponsor]

manoj9april marked the issue as sponsor acknowledged

[manoj9april]

This should be in QA.

[Picodes]

Admin is a multi sig and before updating we will make sure we pause addValidatorKeys function

It may be worth integrating this requirement in the code

[Picodes]

This finding is based on improper configuration by the protocol / admin so falls within QA

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[noamyakov]

@Picodes

Admin is a multi sig and before updating we will make sure we pause addValidatorKeys function

Pausing addValidatorKeys() won't help. This function only deploys new withdraw vaults via deployWithdrawVault(). The issue is with all the withdraw vaults that were already deployed using the previous value of vaultProxyImplementation. When the preDepositOnBeaconChain() function mentioned above (which cannot be paused) internally calls computeWithdrawVaultAddress() looking for the address of such an old withdraw vault that was deployed using the previous value of vaultProxyImplementation, it will return the wrong address - locking out the funds that are being deposited.

In addition to that "exploit" through preDepositOnBeaconChain(), it's worth noting that both the computeWithdrawVaultAddress() and computeNodeELRewardVaultAddress() functions are public view functions intended to be used by users who would like to send funds directly to these contracts (they both have a payable receive() function). Wrong return values pose a huge risk for loss of user funds.

This finding is based on improper configuration by the protocol / admin

This is not correct. It is intended that the admin would change the vaultProxyImplementation address.

I think this issue should be classified as H, not QA.

[JGcarv]

This seems like a non-issue to me.

The operation of deploying a vault and later on calling computeWithdrawVaultAddress is atomic, meaning that no external action, such as changing the implementation, can happen between them.

The only possible call flow, I think, is: permisionlessNodeRegistry.addValidatorKeys() -> permissionlessPool.preDepositOnBeaconChain() -> IVaultFactory.computeWithdrawVaultAddress.

Therefore, I don't see this being an issue

[Picodes]

So for the part "users who would like to send funds directly to these contracts": unless I am missing something, I don't see any reason why users would do this. The receive function is just for the withdrawals.

For the rest, for permissionless pool the flow is direct as shown just above, and for a permissioned pool adding new validators can be paused. Then the admin needs to make sure all newly added validators have been pre-deposited.

Note that the withdrawal vault is also stored here, and that withdrawal credentials of a validator cannot be changed

Overall I think QA is more appropriate here

[noamyakov]

Okay, then what's its grade? A/B/C?

[Picodes]

B! Note that there is only 1 grade across all QA reports by a warden