Twap value is too low in the bridge router and would cause for using wrong prices which leads to wrong accounting

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibBridgeRouter.sol#L111-L141

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibBridgeRouter.sol#L111-L141

    function withdrawalFeePct(uint256 bridgePointer, address rethBridge, address stethBridge) internal view returns (uint256 fee) {
        IBridge bridgeReth = IBridge(rethBridge);
        IBridge bridgeSteth = IBridge(stethBridge);

        // Calculate rETH market premium/discount (factor)
        uint256 unitRethTWAP = OracleLibrary.estimateTWAP(1 ether, 30 minutes, VAULT.RETH_WETH, VAULT.RETH, C.WETH);
        uint256 unitRethOracle = bridgeReth.getUnitDethValue();
        uint256 factorReth = unitRethTWAP.div(unitRethOracle);
        // Calculate stETH market premium/discount (factor)
        uint256 unitWstethTWAP = OracleLibrary.estimateTWAP(1 ether, 30 minutes, VAULT.WSTETH_WETH, VAULT.WSTETH, C.WETH);
        uint256 unitWstethOracle = bridgeSteth.getUnitDethValue();
        uint256 factorSteth = unitWstethTWAP.div(unitWstethOracle);
        if (factorReth > factorSteth) {
            // rETH market premium relative to stETH
            if (bridgePointer == VAULT.BRIDGE_RETH) {
                // Only charge fee if withdrawing rETH
                return factorReth.div(factorSteth) - 1 ether;
            }
        } else if (factorSteth > factorReth) {
            // stETH market premium relative to rETH
            if (bridgePointer == VAULT.BRIDGE_STETH) {
                // Only charge fee if withdrawing stETH
                return factorSteth.div(factorReth) - 1 ether;
            }
        } else {
            // Withdrawing less premium LST or premiums are equivalent
            return 0;
        }
    }

This function is used to bridge fees so as to prevent free arbitrage, fee charged is the premium/discount differential, keep in mind that this function is also called whenever BridgeRouterFacet.withdraw() is called, case here hpwever is that the estimateTwap() hardcodes a secondsAgo value of 30 minutes, but this is too much of a duration for twap, coupling this with the fact that the crypto bull run just kicked in, using a value of 30 minutes would more often than not cause for wrong prices to be used to calculate the fee, essentially breaking the accounting in s.vaultUser[vault][address(this)].ethEscrowed += fee that gets updated in the bridge router facet.

Considering this integration is done with uniswap and not chainlink (i.e chainlink eth feed has a heartbeat of 3600, or deviation of 0.5% so its more often than not an accurate price provider), but here having the twap duration at 30 minutes leave the price updates to be lagging behind.

Impact

Wrong pricing data would be used for calculating the fees leading leak of value or too much value gained since the fee attached to the eth escrow could be over/undervalued.

This also causes unfair loss of assets from the withdrawal attempt if fee is overvalued since the dethAmount is being reduced by this fee too.

NB: Similar bug case is applicable in instances where hardcoded calls are made to estimateWETHInUSDC() with the secondsAgo value as 30 minutes

Recommended Mitigation Steps

Reconsider the value for the TWAP duration, if at all it's still going to be hardcoded, then this duration need to be as short as possible to mirror real current price, but alos not too short so as to protect against manipulation, an optimum duration could be 10/15 minutes.

Assessed type

Timing

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Devoid of structured examples to support your claim.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Insufficient proof