`VaultUser.bridgeCredit` does not decrease when `ETH` is lost due to `Liquidation` or `Redemption`, causing incorrect tally in `BridgeRouter`

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

When a user deposits rETH or stETH through BridgeRouterFacet::deposit using the corresponding bridge (bridgeCreditReth or bridgeCreditSteth), this credit is added to the VaultUser based on the bridge used for deposit as per LibBridgeRouter#L25-L32. This setup was designed to deter users from engaging in arbitrage activities:

File: LibBridgeRouter.sol
21:     function addDeth(uint256 vault, uint256 bridgePointer, uint88 amount) internal {
...
... 
25:         if (vault == VAULT.ONE) {
26:             // Only VAULT.ONE has mixed LST
27:             if (bridgePointer == VAULT.BRIDGE_RETH) {
28:                 VaultUser.bridgeCreditReth += amount;
29:             } else {
30:                 VaultUser.bridgeCreditSteth += amount;
31:             }
32:         }
...
36:     }

Additionally, if the user decides to withdraw using a bridge where they have no credit, they will incur fees as outlined in BridgeRouterFacet#L109-L116:

File: BridgeRouterFacet.sol
100:      */
101:     function withdraw(address bridge, uint88 dethAmount) external nonReentrant {
...
...
106:         uint88 fee;
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 issue arises because this credit is not decreased when a user loses ETH due to liquidation or redemption, resulting in the bridgeCredit becoming completely outdated and the user potentially not being charged the appropriate fees if applicable and allowing the user to arbitrage without penalty.

Proof of Concept

Consider the following scenario:

1. A shorter matches with a bidder. The shorter deposits and is credited in bridgeCreditSteth, say 5e18 stETH.
2. The price crashes, and the order becomes liquidatable, is liquidated, and the shorter loses their collateral 5e18 stETH, but their bridgeCreditSteth does NOT decrease.
3. The shorter then decides to deposit in rETH, and their bridgeCreditReth now amounts to, say, 3e18 rETH.
4. The same shorter sees an arbitrage opportunity and decides to withdraw their 3 rETH using the bridgeCreditSteth. However, since this credit was NOT reduced at the appropriate time (in step 2), the system allows the withdrawal without applying fees as per LibBridgeRouter#L80-L84 because 5e18 stETH > 3e18 amount to withdraw holds true:

File: LibBridgeRouter.sol
039:     function assessDeth(uint256 vault, uint256 bridgePointer, uint88 amount, address rethBridge, address stethBridge)
040:         internal
041:         returns (uint88)
042:     {
...
...
078:         } else {
079:             // Withdraw STETH
080:             creditSteth = VaultUser.bridgeCreditSteth;
081:             if (creditSteth >= amount) {
082:                 VaultUser.bridgeCreditSteth -= amount;
083:                 return 0;
084:             }
...
...
108:         }
109:     }

The following test demonstrates the above scenario:

    // Filename: test/LiquidationPrimary.t.sol:PrimaryLiquidationTest
    function test_bridgeCreditIsNotReducedWhenUserIsLiquidated() public {
        //
        // 1. Assert `sender` does not have `bridgeCreditSteth`
        assertEq(diamond.getVaultUserStruct(vault, sender).bridgeCreditSteth, 0);
        assertEq(diamond.getVaultUserStruct(vault, sender).bridgeCreditReth, 0);
        //
        // 2. Create match orders
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
        fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
        STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
        assertTrue(short.status == SR.FullyFilled);
        //
        // 3. Assert `sender` deposits on `bridgeCreditSteth` and `bridgeCreditReth=0`
        assertEq(
            diamond.getVaultUserStruct(vault, sender).bridgeCreditSteth,
            DEFAULT_PRICE.mulU88(DEFAULT_AMOUNT).mulU88(diamond.getAssetNormalizedStruct(asset).initialCR));
        assertEq(diamond.getVaultUserStruct(vault, sender).bridgeCreditReth, 0);
        skip(1 days);
        // Price drops in order to be liquidatable
        _setETH(1000 ether);
        //
        // 4. Liquidatation occurs and the short is closed
        uint88 amount = short.ercDebt + short.ercDebt.mulU88(diamond.getAssetStruct(asset).ercDebtRate - short.ercDebtRate);
        fundLimitAskOpt(DEFAULT_PRICE, amount, receiver); 
        vm.prank(extra);
        diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);
        short = getShortRecord(sender, C.SHORT_STARTING_ID);
        assertTrue(short.status == SR.Closed);
        //
        // 5. The `sender's bridgeCreditSteth` has not decreased even when the `sender` lost his `eth`. bridgeCreditReth is still 0
        assertEq(
            diamond.getVaultUserStruct(vault, sender).bridgeCreditSteth,
            DEFAULT_PRICE.mulU88(DEFAULT_AMOUNT).mulU88(diamond.getAssetNormalizedStruct(asset).initialCR));
        assertEq(diamond.getVaultUserStruct(vault, sender).bridgeCreditReth, 0);
        //
        // 6. Shorter deposits to rETH, now `bridgeCreditReth=3 ether`
        depositReth(sender, 3 ether);
        assertEq(diamond.getVaultUserStruct(vault, sender).bridgeCreditReth, 3 ether);
        //
        // 7. Shorter withdraw his `3e18 rETH` using the `stETH` bridge, it receives the entire amount in `stETH` without penalty
        uint256 balanceBeforeWithdraw = reth.balanceOf(sender); 
        vm.prank(sender);
        diamond.withdraw(_bridgeSteth, 3 ether);
        assertEq(balanceBeforeWithdraw + 3 ether, steth.balanceOf(sender));
    }

Tools used

Manual review

Recommended Mitigation Steps

Update the corresponding bridgeCredit when the eth decreases due to liquidation or redemption.

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 (sponsor) disputed

[ditto-eth]

Not worried about this for two reasons:

• This scenario doesn't result from intentional behavior, would have to lose ETH in order to "arbitrage" in the first place
• The user being rewarded will have more ETH without the credit so will pay fees to the system

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid