DittoETH

Summary

Downcast from uint256 to uint8 of the vault parameter in the OwnerFacet::createBridge function may lead to the wrong assignment of the vault parameter (if it is more than 255) to the vault element in the bridge struct. This downcasted element vault of the bridge struct is used in all core functions of the BridgeRouterFacet contract to select a vault, based on which accounting state the execution of any of these functions will be performed: deposit, depositEth, withdraw, unstakeEth, withdrawTapp. The same vault element of the bridge struct is also used in the removeBridge function of the OwnerFacet contract to remove the bridge from the existing vault, but due to the wrong assignment of the downcasted value of the vault element in the bridge struct the bridge actually will not be deleted, even though the function will be executed.

Vulnerability Details

Firstly, we should take a look at the bridge struct (DataTypes contract, lines 168-173):

Then, at the mapping vaultBridges (AppStorage contract, line 24):

Then, at the function createBridge, where we can see an unsafe downcast from uint256 to uint8 of the parameter vault (OwnerFacet contract, lines 280-281):

Then, we shall examine the core functions of BridgeRouterFacet contract – all of the core functions follow the same type of execution: 1) if rethBridge or stethBridge is used – then we are using Vault.Carbon, whose uint index is 1; 2) if we are using another bridge – then the vault is chosen based on the extracted value of the vault element from the bridge struct:

The situation in our scope is the second one: if amount of vaults operating in the protocol exceeds the borders of uint8 max value (i.e. >255) and a bridge was created for any of these vaults, then the bridge struct will be incorrectly organized because of the downcast. For example: bridge created for the vault n.256 – the vault element in the bridge struct is set to 0, for n.257 – to 1, for n.258 – to 2 and so on (I realize that this maybe an unrealistic case – still documentation says that there is a possibility of adding new vaults without any given info about limitations, so with the growth of the protocol this condition might be reached. Also in the already stated mapping vaultBridges the key vault is set to uint256, implicating that the amount of vaults can be more than uint8, also when used in functions vault parameter is never being used as uint8).
As a result, all functions in the BridgeRouterFacet executed through a non-standart bridge will be executed on the wrongly vault based on the extracted value of the downcasted vault element from the bridge struct, resulting in a serious accounting issues.
Also, another major issue is connected with the deleteBridge function of the OwnerFacet contract, because the selection of the vault, from which the bridge should be deleted, is based on the extraction of the vault element from the bridge struct (line 288):

As a result, even though the function will be executed, actually the bridge will not be deleted for the required vault, since the vault element in the bridge struct was wrongly assigned.

Impact

1. The accounting of the existing vaults during the execution of any asset-managing functions in the BridgeRouterFacet contract that were meant to operate with vaults >255 is going to be changed, resulting in serious accounting issues for the protocol;

2. There will be no possibility to perform accounting operations on any of the vaults that is more than 255, since the vault parameter in the bridge struct will never point on them, even though the bridge itself will be connected;

3. With vaults, which can be divided by 256 (i.e. 256, 512, 786 etc...), it will be impossible to execute any operations via the BridgeRouterFacet because of the onlyValidBridge modifier, since the s.bridge[bridge].vault value will be 0, which will cause functions to revert;

4. There will be no possibility to delete bridges for newly created vaults with index >255.

Tools Used

Manual Review, Foundry

Proof of concept

Actors:

Admin: – creates a bridge for a vault, whose tracking number is more than 255 (for the clearer explanation I will take the number 257); After some time Admin decides to remove that bridge.
User: - via the newly created bridge decides to use any of the BridgeRouterFacet function, e.g. deposit:

Overview

1. The new bridge has been added by the admin to a vault n.257; As a result, the bridge for a vault n.257 will be created (we can check through the output of the getBridges function), but in a bridge struct itself value of the vault element will be set to 1. In order to illustrate this I have added this test to Owner.t.sol contract (using forge test --match-contract Owner --match-test testCreateBridge_MoreThanUint8 -vvvv):

Here are the results:

As we can see in the CreateBridge emitted event the bridge struct for the provided bridge address has been organized that way - withdrawalFee: 1000 (10%), unstakeFee: 0, vault: 1 (not 257), even though the bridge was actually connected to the required vault n.257, as we can see via the returned value of the getBridges function.
2. So, when the user calls the deposit function via the newly created bridge, it will follow the "vault = s.bridge[bridge].vault scenario", the chosen vault to perform the rest of the deposit function will be the first, the Vault.CARBON vault, not the vault n.257 as it should have been;
3. After that, all accounting operations will be performed in the CARBON vault

resulting in wrong accounting.
4. Moreover, if the bridge was connected to the vault, which number can be divided by 256 (e.g. 256, 512, 768) – not a single operation via that bridge can be executed, since it will not pass the onlyValidBridge modifier (AppStorage contract, lines 123-126)

because the value of the vault parameter will be downcasted to 0 during the execution of the createBridge function and this 0 value will be assigned to the vault element in the bridge struct.
In order to illustrate this issue, we can just change the 257 value to 256 in our test:

As we can see, the vault element in the bridge struct is 0. So, being called in any of the functions, that contain onlyValidBridge modifier, the function will revert.

This issue can be applied to all other core functions of the BridgeRouterFacet contract, because all of them contain the stated pattern:

(deposit function was just taken as an example). Depending on the calling function, this issue also makes the private maybeUpdateYield (if deposit or depositEth is called) function to wrongly update yield and for the _ethConversion (if withdraw, unstakeEth or withdrawTapp is called) it makes it return wrong values.
Finally, the previously stated pattern is also used in the deleteBridge function of the OwnerFacet contract, which means that the admin will not be able to delete a bridge for any vaults, which index will be more than 255. In order to illustrate this issue, I have added this test to the Owner.t.sol contract:

Here are the results:

As we can see, the returned value of the getBridges function have not changed after the execution of the deleteBridge function, even though the function has been executed.

Recommendations

Reorganize the bridge struct by changing the uint8 parameter to uint256 or any uint up to 232 (256 - 8 (unstakeFee) - 16(withdrawalFee)) if there is still need to occupy only 1slot for the bridge struct (this way it will be more than enough vaults) and remove the unsafe typecast in the createBridge function of the OwnerFacet contract.