L1::xRenzoBridge and L2::xRenzoBridge uses the block.timestamp as dependency, which can cause issue.

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/L1/xRenzoBridge.sol#L216 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/L2/xRenzoDeposit.sol#L345

Vulnerability details

Title

L1::xRenzoBridge and L2::xRenzoBridge uses the block.timestamp as dependency, which can cause issue.

Line of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/L1/xRenzoBridge.sol#L216

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/L2/xRenzoDeposit.sol#L345

Impact

In L1::xRenzoBridge the block.timestamp from L1 is encoded and sent to L2. When the message is delivered from L1 to L2 with xRenzoBridge::_updatePrice(), the function checks the block.timestamp like this:

solidity📋
    if (_timestamp > block.timestamp) {
            revert InvalidTimestamp(_timestamp);
        }

This check is done to not allow future timestamps for updating the price But the timestamps between two chains L1 and L2 are different for chain like Arbitrum as there's a possibility that the sequencer fails to post batches on the parent chain (for example, Ethereum) for a period of time. According to the Arbitrum doc:

Timestamp boundaries of the sequencer

As mentioned, block timestamps are usually set based on the sequencer's clock. Because there's a possibility that the sequencer fails to post batches on the parent chain (for example, Ethereum) for a period of time, it should have the ability to slightly adjust the timestamp of the block to account for those delays and prevent any potential reorganisations of the chain. To limit the degree to which the sequencer can adjust timestamps, some boundaries are set, currently to 24 hours earlier than the current time, and 1 hour in the future.

So the issue is that timestamp validation for _updatePrice() won't be effective and can reject validation both l2 tiimestamp is not related to l1 timestamp

https://docs.arbitrum.io/build-decentralized-apps/arbitrum-vs-ethereum/block-numbers-and-time#block-timestamps-arbitrum-vs-ethereum

Block timestamps on Arbitrum are not linked to the timestamp of the L1 block. They are updated every L2 block based on the sequencer's clock. These timestamps must follow these two rules:

Must be always equal or greater than the previous L2 block timestamp Must fall within the established boundaries (24 hours earlier than the current time or 1 hour in the future). More on this below.

Furthermore, for transactions that are force-included from L1 (bypassing the sequencer), the block timestamp will be equal to either the L1 timestamp when the transaction was put in the delayed inbox on L1 (not when it was force-included), or the L2 timestamp of the previous L2 block, whichever of the two timestamps is greater.

Proof of Concept

xRenzoBridge::sendPrice is used to send the price feed from L1 to L2. The bytes memory _callData = abi.encode(exchangeRate, block.timestamp); encodes the block.timestamp of L1.

solidity📋
    function sendPrice(
        CCIPDestinationParam[] calldata _destinationParam,
        ConnextDestinationParam[] calldata _connextDestinationParam
    ) external payable onlyPriceFeedSender nonReentrant {
        // call getRate() to get the current price of ezETH
        uint256 exchangeRate = rateProvider.getRate();

@>      bytes memory _callData = abi.encode(exchangeRate, block.timestamp);

        ...
        ...
    }

Now if we see this function xRenzoBridge::_updatePrice(), the timestamp here is compared to the timestamp of L2 is not effective to check the older price timestamps is greater than block.timestamp as both of these timestamps of L1 and L2 are different.

solidity📋

    function _updatePrice(uint256 _price, uint256 _timestamp) internal {
        // Check for 0
        if (_price == 0) {
            revert InvalidZeroInput();
        }

        // Check for price divergence - more than 10%
        if (
            (_price > lastPrice && (_price - lastPrice) > (lastPrice / 10)) ||
            (_price < lastPrice && (lastPrice - _price) > (lastPrice / 10))
        ) {
            revert InvalidOraclePrice();
        }

        // Do not allow older price timestamps
        if (_timestamp <= lastPriceTimestamp) {
            revert InvalidTimestamp(_timestamp);
        }

        // Do not allow future timestamps
@>      if (_timestamp > block.timestamp) {
            revert InvalidTimestamp(_timestamp);
        }

        // Update values and emit event
        lastPrice = _price;
        lastPriceTimestamp = _timestamp;

        emit PriceUpdated(_price, _timestamp);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

remove the timestamp check in in l2 update rate

Assessed type

Timing