Lambda Audit Report

# Lambda Audit Report ## High - `EthStrategy`: Cannot receive ETH The `EthStrategy` contract does not have a `receive` function and therefore cannot receive ETH when unstaking from Lido. ### Recommendation Add a `receive` function. ## High - `StakingManager`: No way to recover ETH There are two ways how ETH can end up in the `StakingManager` contract: - Removing a strategy, which calls `strategy.withdraw`, but does not send the received ETH anywhere. - Specifically for `EthStrategy`: Calling `ownerWithdraw`, which sends ETH directly to the staking manager. However, the staking manager contract does not have a method to recover this ETH and it will be lost. ### Recommendation It should be defined what happens with these funds. Depending on how centralized the flow should be, one possibility would be to let the owner withdraw it. ## Medium - ewETH may become undercollaterized because of slashing events in strategies `EdgelessDeposit.mintEthBasedOnStakedAmount` allows to mint up to the current asset total of the strategies. However, this value may decrease in the future, making ewETH undercollaterized. More specifically for the LIDO strategy (which is the only at the moment), it can happen because of slashing events, but future strategies may have some different conditions. ### Recommendation Completely avoiding this is not really possible in practice, but the impact could be alleviated. For instance, an insurance pool could be introduced or the losses could be distributed among all other users (unlike in the current design, where the last users will bear all the losses, incentivizing withdrawing everything as soon as this happens). ## Medium - `EdgelessDeposit`: Redemptions may use funds of other users The function `withdrawEth` calls `StakingManager._withdrawEth`, which calls `strategy.withdraw` and sends to the deposit contract only the funds that it received. This may be lower than the funds that were requested (for the ETH strategy, this happens because the tokens are staked). In such a case, the withdrawal will typically still succeed, but use the ETH that other users deposited. Because the deposit contract is then technically undercollaterized, this may cause a bank run and the last redemptions will then fail (until an admin manually unstakes). ### Recommendation With the current design, this is hard to avoid completely. To not have situations where the deposit contract is temporarily undercollaterized, one possibility would be to revert when the withdrawal does not result in enough funds (although it is not clear if that is better because no users could withdraw in that case). A different design that some platforms (e.g. eBTC) use is to not integrate with LIDO directly, but to perform ETH / stETH swaps (e.g., on Uniswap). This allows immediate conversion of stETH to ETH and there is no need for owner interventions. ## Medium - `EthStrategy.claimLidoWithdrawals` time complexity The function `LIDO_WITHDRAWAL_ERC721.findCheckpointHints` performs a binary search for every request ID and therefore has a time complexity of O(numRequests * log(numCheckpoints)). While `numRequests` is admin-controlled (and splitting IDs would be possible), `numCheckpoints` grows continuously. At some point, this may therefore run out of gas, making withdrawals impossible. ### Recommendation There are two approaches to solve this: - Passing in the hints (this would be more efficient anyways because it can then be calculated off-chain without any gas constraints, so the owner saves gas when claiming) - Not hard-coding the start (currently always 1) for `findCheckpoints`, but making it configurable. ## Low - `WrappedToken`: Unnecessary events for mint / burn The `mint` and `burn` functions both emit a dedicated event. Because they internally call `_mint` / `_burn`, which already does emit a special `Transfer` event in these scenarios, these events are unnecessary and could be removed to save gas. ### Recommendation Consider removing the events. ## Low - Unused parameters There are a few parameters that are not used and can be removed: - `EdgelessDeposit.initialize`: The `_staker` argument is not used (except for checking that it is non-zero). I recommend removing it. - `StakingManager.stake`: The function has an argument `asset`, but only supports staking ETH (at the moment). The argument is ignored except for the event. While this interface may have been chosen to be able to add new assets in the future, the current implementation is pretty confusing because it would technically support passing in an arbitrary asset that is then also emitted in the event. In practice, this can never happen with the current deposit contract (that is only allowed to call this function) because it only passes in the ETH address, but if the idea is to support more assets in the future, you might want to consider already adding validation for this (i.e. reverting for non-supported assets). Moreover, the interface is not consistent because `withdraw` does not have an `asset` parameter, so if the reason for this argument was to have a future-proof interface, this is not the case because `withdraw` needs to be changed anyways.