DittoETH

Summary

The onlyValidBridge() modifier would be check whether or not a given bridge address would be a valid bridge address by confirming the given bridge address is stored into the AppStorage.bridge storage (s.bridge[bridge].vault).

If an underlying LST protocol (i.e. Lido, Rocket Pool) used for an existing bridge in the DittoETH protocol would be exploited due to a malicious attack, the existing bridge address is supposed to be removed from the AppStorage.bridge storage (s.bridge[bridge].vault).
However, there is no function to remove an existing bridge address from the AppStorage.bridge storage (s.bridge[bridge].vault).

This allow a user to still be able to deposit their native ETH into the underlying LST protocol (i.e. Lido, RocketPool) via the bridge even if the underlying LST protocol of the bridge would be exploited due to a malicious attack.
This lead to losing user's native ETH-sent to the underlying LST protocol-exploited to deposit.

Because the bridge address can always pass the onlyValidBridge() modifier due to lacking of the function to remove an existing bridge address from the s.bridge[bridge].vault storage.

Vulnerability Details

According to the "BridgeRouter" part in the documentation, in the initial launch, the stETH (Lido) bridge and the rETH (Rocket Pool) bridge would be deployed as a valid bridge in the DittoETH protocol like this:

The initial launch will support stETH (Lido) and rETH (Rocket Pool), with designs on supporting other quality ETH LSTs in the future.

Within the Modifiers contract, the onlyValidBridge() modifier would be defined to check whether or not a given bridge address would be a valid bridge address, which is stored into the AppStorage.bridge storage (s.bridge[bridge].vault) like this:
https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/libraries/AppStorage.sol#L124

Within the BridgeRouterFacet contract, the onlyValidBridge() modifier would be used in the following functions in order to check whether or not a given bridge address would be a valid bridge address:

• BridgeRouterFacet#depositEth()

• BridgeRouterFacet#unstakeEth()
  https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/BridgeRouterFacet.sol#L71
  https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/BridgeRouterFacet.sol#L119

If an underlying LST protocol (i.e. Lido, Rocket Pool) used for an existing bridge in the DittoETH protocol would be exploited due to a malicious attack, the existing bridge address is supposed to be removed from the AppStorage.bridge storage (s.bridge[bridge].vault).
However, there is no function to remove an existing bridge address from the AppStorage.bridge storage (s.bridge[bridge].vault).

This allow a user to still be able to deposit their native ETH into the underlying LST protocol (i.e. Lido, RocketPool) via the bridge even if the underlying LST protocol of the bridge would be exploited due to a malicious attack.
This lead to losing user's native ETH-sent to the underlying LST protocol-exploited to deposit.

Because the bridge address can always pass the onlyValidBridge() modifier due to lacking of the function to remove an existing bridge address from the AppStorage.bridge storage (s.bridge[bridge].vault).

Impact

This lead to losing user's native ETH-sent to the underlying LST protocol-exploited to deposit.

Tools Used

• Foundry

Recommendations

Within the Modifier contract, consider adding a function for an admin user to remove an existing bridge address from the AppStorage.bridge storage (s.bridge[bridge].vault) like this: