Users receiving extra `ETH`, whether as `Liquidator` or `Redeemer`, will incur fees when withdrawing

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/BridgeRouterFacet.sol#L101

Vulnerability details

Impact

The liquidator will receive ETH when the liquidation is successful in PrimaryLiquidationFacet#L220 or PrimaryLiquidationFacet#L223. Similarly, when a redemption can be claimed, the redeemer will also receive ETH (RedemptionFacet#L331).

File: PrimaryLiquidationFacet.sol
209:     function _liquidationFeeHandler(MTypes.PrimaryLiquidation memory m) private {
...
...
218:         if (TAPP.ethEscrowed >= callerFee) {
219:             TAPP.ethEscrowed -= callerFee;
220:             VaultUser.ethEscrowed += callerFee;
221:         } else {
222:             // Give caller (portion of?) tappFee instead of gasFee
223:             VaultUser.ethEscrowed += callerFee - m.gasFee + tappFee;
224:             m.totalFee -= m.gasFee;
225:             TAPP.ethEscrowed -= m.totalFee;
226:         }
227:     }

File: RedemptionFacet.sol
310:     function claimRedemption(address asset) external isNotFrozen(asset) nonReentrant {
...
...
331:         redeemerVaultUser.ethEscrowed += totalColRedeemed;
...
...
334:     }

The issue arises when the user, whether a liquidator or redeemer, incurs fees when withdrawing the ETH. This occurs because in BridgeRouterFacet, if there is insufficient credit, fees are charged (BridgeRouterFacet#L109-L116).

File: BridgeRouterFacet.sol
101:     function withdraw(address bridge, uint88 dethAmount) external nonReentrant {
...
...
107:         if (vault == VAULT.ONE) {
108:             uint88 dethAssessable = vault.assessDeth(bridgePointer, dethAmount, rethBridge, stethBridge);
109:             if (dethAssessable > 0) {
110:                 uint256 withdrawalFeePct = LibBridgeRouter.withdrawalFeePct(bridgePointer, rethBridge, stethBridge);
111:                 if (withdrawalFeePct > 0) {
112:                     fee = dethAssessable.mulU88(withdrawalFeePct);
113:                     dethAmount -= fee;
114:                     s.vaultUser[vault][address(this)].ethEscrowed += fee;
115:                 }
116:             }
117:         }
...
...
123:     }

The liquidator will end up paying fees based on the withdrawal amount and the bridge the liquidator intends to use, which may affect the final amount of ETH received. In some cases, it might not be convenient for the liquidator to pay those fees if the received collateral was minimal.

Proof of Concept

Consider the following scenario:

1. UserA is solely a liquidator and has no ETH deposited in any bridge, resulting in ethEscrowed=0, bridgeCreditReth=0, and bridgeCreditSteth=0.
2. UserA receives ETH through successful liquidations, increasing their ethEscrowed.
3. UserA decides to withdraw the ETH; however, since he has no credit in bridgeCreditReth or bridgeCreditSteth, they will be charged a fee.

File: BridgeRouter.t.sol
638:     function testFork_liquidatorPayFeesWhenWithdraw() public {
639:         // Deposit some stETH and `rETH` to the sender in order to have `eth` in the system
640:         bridgeWithdrawSetup(); 
641:         vm.stopPrank();
642:         address liquidator = address(12212);
643:         //
644:         // 1. Assert `sender` does not have `bridgeCreditSteth` and no ethescrow
645:         assertEq(diamond.getVaultUserStruct(vault, liquidator).bridgeCreditSteth, 0);
646:         assertEq(diamond.getVaultUserStruct(vault, liquidator).bridgeCreditReth, 0);
647:         assertEq(diamond.getVaultUserStruct(vault, liquidator).ethEscrowed, 0);
648:         //
649:         // 2. Simulate the liquidator obtains some `ether` from liquidations
650:         diamond.setEthEscrowed(liquidator, 33 ether);
651:         assertEq(diamond.getVaultUserStruct(vault, liquidator).bridgeCreditSteth, 0);
652:         assertEq(diamond.getVaultUserStruct(vault, liquidator).bridgeCreditReth, 0);
653:         assertEq(diamond.getVaultUserStruct(vault, liquidator).ethEscrowed, 33 ether);
654:         //
655:         // 3. Liquidator `withdraws` using the `stETH` bridge and the `stETH` deposited to the liquidator is less than 33 ether, he paid some fees
656:         // in order to withdraw
657:         vm.prank(liquidator);
658:         diamond.withdraw(_bridgeSteth, 33 ether);
659:         assertGt(33 ether, steth.balanceOf(liquidator)); // `stETH` is less than `33 ether`
660:     }

Tools used

Manual review

Recommended Mitigation Steps

Liquidators should be able to withdraw ETH without incurring fees. It's necessary to consider transferring the bridgeCredit from the liquidated user to the liquidator, and the same applies to redemptions.

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Will let sponsor review the coded POC and its validity.

[c4-sponsor]

ditto-eth marked the issue as disagree with severity

[c4-sponsor]

ditto-eth (sponsor) disputed

[c4-sponsor]

ditto-eth marked the issue as agree with severity

[ditto-eth]

This is intentional, the bridge credit system is overly conservative in protecting against arbitrage. The bridge credit system is not only a "reward" for fee-free withdrawals but also a restriction to protect bridge makeup: https://dittoeth.com/technical/arbitrage#credit-system

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid