Pull request #1568 review

Gadze Finance SEZC requested an isolated review of the changes made to LiquidityPool in pull request (PR) #1568↗. The PR is meant to introduce an alternative flow where the LiquidityPool contract acts as the owner of the B-NFT. This section documents our observations regarding that PR.

We note that LiquidityPool was not part of the original scope of this audit and that this subengagement was limited to a review of the changes (and possible issues) introduced by the PR in question, in commits , , and (the HEAD of the PR at the time of writing). Note that the PR also contains merge commit , which includes changes not related to the scope of this review.

Introduction of batchDepositWithLiquidityPoolAsBnftHolder and batchRegisterWithLiquidityPoolAsBnftHolder

Gadze Finance SEZC introduced an additional deposit and registration flow that allows to use the liquidity pool contract as the B-NFT holder. The new flows are intended to be used as an exclusive alternative to the regular flows. The contract has a variable that determines if the liquidity pool should operate as the B-NFT holder (isLpBnftHolder). It is initialized as false (allowing to use the regular flow) and can be updated only by the contract administrators.

As also noted in a comment, updateBnftMode should not be called while a deposit registration is in progress. Gadze Finance SEZC is aware of this and has expressed the intention to use updateBfntMode after pausing the contract and verifying that no deposits are pending. While pausing the contract to verify that no deposits are pending is not ideal, this design does not fundamentally change the trust users have to put in the contract owners, because the contracts are upgradable.

To create the new flow, the previous batchDepositAsBnftHolder and batchRegisterAsBnftHolder functions were renamed to _batchDeposit and _batchRegister, respectively, and changed to internal. The logic of the functions was also adjusted to account for the new requirements. The now internal functions are invoked by two public functions acting as entry points for the two flows.

_batchDeposit

The logic for batch depositing was changed to now take a parameter that specifies how much ETH must be included with the calling message. With the regular flow, the caller is required to provide 2 ETH per validator. With the new flow, the caller is not required to provide any ETH.

_batchRegister

The batch-registration function was updated to take the recipient of the B-NFT as an argument. When the function is invoked using the previous flow, the B-NFT recipient is msg.sender. When it is invoked using the new flow where the LP acts as B-NFT holder, the address of the pool is used instead.

Notable unrelated changes

When comparing commit with commit (the HEAD of PR #1568 at the time of writing), we noted the following unrelated changes, which we document for completeness.

Transfer-flow changes. Transfers to the staking manager contract are now performed at the time of the deposit actions instead of when a validator is registered. We were informed that this change is part of a work-in-progress modification to the protocol.

Replacement of _sendFund. The _sendFund function used to transfer ETH was replaced by a manual low-level call (with a check on the call success). The _sendFund function performed an additional check that ensured the balance after the call equals the balance before the call minus the transferred amount. The removal of this check technically allows the recipient of the transfer to transfer any amount of ETH back into the LiquidityPool contract. We do not believe this creates a security issue as the contract balance can only be increased.

Initialization of fundStatistics. The contract initializer now sets fundStatistics[...].numberOfValidators to 1 for keys SourceOfFunds.EETH and SourceOfFunds.ETHER_FAN. We believe this was done to simplify allocateSourceOfFunds, which no longer handles the special case of numberOfValidators == 0.