Summary
If the dao removes a bridge for any (non-malicious) reason, user's deposited tokens for that bridge will be lost.
Vulnerability Details
In the OwnerFacet.sol the dao of the system has the option to remove a bridge by calling the deleteBridge() function. There is no check if there are any assets in the bridge. Also users may deposit funds in the bridge during the voting period.
POC
Add the following function in the BridgeRouter.t.sol
function test_DeleteBridgeWithAssets() public {
console.log("Sender ethEscrowed in vault 2 before deposit: ", diamond.getVaultUserStruct(2, sender).ethEscrowed);
deal(_rethA, sender, 10000 ether);
vm.startPrank(sender);
uint88 deposit1 = 1000 ether;
uint88 withdrawAmount = 100 ether;
diamond.deposit(_bridgeRethToBeDeleted, deposit1);
console.log("Sender ethEscrowed in vault2 after deposit: ", diamond.getVaultUserStruct(2, sender).ethEscrowed);
diamond.withdraw(_bridgeRethToBeDeleted, withdrawAmount);
console.log("Sender ethEscrowed after withdraw: ", diamond.getVaultUserStruct(2, sender).ethEscrowed);
vm.stopPrank();
console.log("Balance of reth in the bridgeRethToBeDeleted: ", rethA.balanceOf(_bridgeRethToBeDeleted));
vm.startPrank(owner) ;
diamond.deleteBridge(_bridgeRethToBeDeleted);
vm.stopPrank();
vm.startPrank(sender);
vm.expectRevert();
diamond.withdraw(_bridgeRethToBeDeleted, withdrawAmount);
console.log("Balance of reth in the bridgeRethToBeDeleted: ", rethA.balanceOf(_bridgeRethToBeDeleted));
vm.stopPrank();
}
In order to run this test, you also have to add
rethA.approve(
_bridgeRethToBeDeleted,
0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
);
to setUp() function of the BridgeROuter.t.sol contract
In DeployHelper.sol another bridge and vault have to be added in order for the test to run:
IBridge public bridgeRethToBeDeleted;
address public _bridgeRethToBeDeleted;
IAsset public zethToBeDeletedVault;
address public _zethToBeDeletedVault;
IRocketStorage public rocketStorageA;
address public _rocketStorageA;
IRocketTokenRETH public rethA;
address public _rethA;
Add the following to deployContracts() function
if (chainId == 31337) {
_immutableCreate2Factory = deployCode("ImmutableCreate2Factory.sol");
if (isMock) {
_steth = deployCode("STETH.sol");
_unsteth = deployCode("UNSTETH.sol", abi.encode(_steth));
_rocketStorage = deployCode("RocketStorage.sol");
_reth = deployCode("RocketTokenRETH.sol");
reth = IRocketTokenRETH(_reth);
_ethAggregator = deployCode("MockAggregatorV3.sol");
_rocketStorageA = deployCode("RocketStorage.sol");
_rethA = deployCode("RocketTokenRETH.sol");
rethA = IRocketTokenRETH(_rethA);
}
rocketStorage = IRocketStorage(_rocketStorage);
rocketStorageA = IRocketStorage(_rocketStorageA);
steth = ISTETH(_steth);
unsteth = IUNSTETH(payable(_unsteth));
ethAggregator = IMockAggregatorV3(_ethAggregator);
}
_zethToBeDeletedVault = factory.safeCreate2(
salt,
abi.encodePacked(
vm.getCode("Asset.sol:Asset"), abi.encode(_diamond, "Zebra ETH Two", "ZETHT")
)
);
_bridgeRethToBeDeleted = factory.safeCreate2(
salt,
abi.encodePacked(
vm.getCode("BridgeReth.sol:BridgeReth"),
abi.encode(_rocketStorageA, _diamond)
)
);
bridgeRethToBeDeleted = IBridge(_bridgeRethToBeDeleted);
MTypes.CreateVaultParams memory vaultParams;
vaultParams.zethTithePercent = 10_00;
vaultParams.dittoMatchedRate = 1;
vaultParams.dittoShorterRate = 1;
diamond.createVault({zeth: _zeth, vault: Vault.CARBON, params: vaultParams});
MTypes.CreateVaultParams memory vaultParamsTwo;
vaultParamsTwo.zethTithePercent = 9_00;
vaultParamsTwo.dittoMatchedRate = 1;
vaultParamsTwo.dittoShorterRate = 1;
zethToBeDeletedVault = IAsset(_zethToBeDeletedVault);
diamond.createVault({zeth: _zethToBeDeletedVault, vault: 2, params: vaultParamsTwo});
STypes.Vault memory carbonVaultConfigTwo = diamond.getVaultStruct(2);
assertEq(carbonVaultConfigTwo.zethTithePercent, 9_00);
diamond.createBridge({
bridge: _bridgeRethToBeDeleted,
vault: 2,
withdrawalFee: 150,
unstakeFee: 0
});
if (isMock) {
rocketStorage.setDeposit(_reth);
rocketStorage.setReth(_reth);
rocketStorageA.setDeposit(_rethA);
rocketStorageA.setReth(_rethA);
_setETH(4000 ether);
}
To run the test use forge test -vvv --mt test_DeleteBridgeWithAsset
Impact
User's deposited RETH or STETH in that bridge will be lost, as the user doesn't have the option to withdraw them. Because the withdraw functions can only be called trough the Diamond.sol
Tools Used
Manual Review
Recommendations
In deleteBridge() make sure that the contract doesn't have any assets
