# 0xWeiss Audit Report

## [H-1] LIDO withdrawals won't work

Description:

When you claim a LIDO withdrawal, you are exchanging your STETH for ETH again. If you go to LIDOs code:

https://etherscan.deth.net/address/0x889edC2eDab5f40e902b864aD4d7AdE8E412F9B1#writeProxyContract

You can see that an external call towards msg.sender is done to send back the ether.

function _sendValue(address _recipient, uint256 _amount) internal {
if (address(this).balance < _amount) revert NotEnoughEther();

// solhint-disable-next-line
(bool success,) = _recipient.call{value: _amount}("");
if (!success) revert CantSendValueRecipientMayHaveReverted();
}

This msg.sender is the EthStrategy contract which does not have a receive function.

Recommendation:

Add a receive function in the EthStrategy contract.
+ receive() external payable { }

## [H-2] Removing strategies will fail most of the time

Description:

When you claim a LIDO withdrawal, you are exchanging your STETH for ETH again. If you go to LIDOs code:

https://etherscan.deth.net/address/0x889edC2eDab5f40e902b864aD4d7AdE8E412F9B1#writeProxyContract

You can see that an external call towards msg.sender is done to send back the ether.

function _sendValue(address _recipient, uint256 _amount) internal {
if (address(this).balance < _amount) revert NotEnoughEther();

// solhint-disable-next-line
(bool success,) = _recipient.call{value: _amount}("");
if (!success) revert CantSendValueRecipientMayHaveReverted();
}

This msg.sender is the EthStrategy contract which does not have a receive function.

Recommendation:

Add a receive function in the EthStrategy contract.
+ receive() external payable { }

## [M-1] UUPSUpgradeable is missing

Description:

On the Readme says that - **Edgeless Deposit Contract**: This contract is UUPS Upgradeable by the owner. the Edgeless Deposit Contract is UUPS upgradeable, though it does not implement the UUPSUpgradeable import from OZ.

Recommendation:

Add the following to the inheritance list:

+ contract EdgelessDeposit is UUPSUpgradeable, Ownable2StepUpgradeable {
Check more about proxies: https://docs.openzeppelin.com/contracts/5.x/api/proxy

## [M-2] Attacker can take over the implementation contract

Description:

When using UUPS proxies, it is mandatory that the implementation contract can't be left un-initialized becuase and attacker could take over the contract and either upgrade or selfdestruct.

Recommendation:

Add the following to all the implementation contracts in the codebase.

+/// @custom:oz-upgrades-unsafe-allow constructor
+ constructor() {
+ _disableInitializers();
+ }
Check more about proxies: https://docs.openzeppelin.com/contracts/5.x/api/proxy

## [M-3] Corrupted upgradeability pattern

Description:

The contracts EdgelessDeposit, StakingManager, and EthStrategy , do not have gap storage implemented.

Thus, adding new storage variables to any of these inherited contracts can potentially overwrite the beginning of the storage layout of the child contract. causing critical misbehaviors in the system.

Recommendation:

Consider defining an appropriate storage gap in each upgradeable parent contract at the end of all the storage variable definitions as follows:

uint256[50] __gap; // gap to reserve storage in the contract for future variable additions`

Note, ideally you would mantain a gap (counting current state variables) of 50 slots.

Therefore is you have 3 slots occupied, the gap should be of 47 slots.

You also can check the following report for reference:

https://blog.openzeppelin.com/notional-audit/

## [L-1] Event state can be incorrect

Description:There are multiple cases in the codebase of event re-entrancy, meaning that events are emitted after external calls, which can re-enter in different parts of the codebase and be inaccurate. While this s somehow accepted by most protocols, it is nevertheless still wrong.

Recommendation:

Move the events before the external calls in functions like withdrawEth in EdgelessDeposit:

+ emit WithdrawEth(msg.sender, to, amount, amount);
(bool success, bytes memory data) = to.call{ value: amount }("");
if (!success) revert TransferFailed(data);
- emit WithdrawEth(msg.sender, to, amount, amount);

## [GAS] Redundant initialization on for loops and variables declarations:

On the whole codebase there are redundant initializations, an example:

function requestLidoWithdrawal(uint256[] calldata amount)
external
onlyOwner
returns (uint256[] memory requestIds)
{
uint256 total = 0; //@audit-issue G no need for init this vars, and the ++i
for (uint256 i = 0; i < amount.length; i++) {
total += amount[i];
}
LIDO.approve(address(LIDO_WITHDRAWAL_ERC721), total);
requestIds = LIDO_WITHDRAWAL_ERC721.requestWithdrawals(amount, address(this));
emit RequestedLidoWithdrawals(requestIds, amount);
}

- uint256 total = 0; should be uint256 total;
- for (uint256 i = 0; i < amount.length; i++) should be for (uint256 i; i < amount.length; ++i)