Skip to content

Commit

Permalink
Merge branch 'main' into trusted-clones
Browse files Browse the repository at this point in the history
* main:
  Audited Release v1.0.0
  fix ci, add more missing deploys
  more chains, add dprc, provider priority, various cleanup
  - Update Scroll/Base to default to Alchemy
  - Additional debug output when running tests
  few more networks
  - Update Linea mainnet to latest LineaRollup
  moving machines
  - Added details for metric tracking with gateway operations
  • Loading branch information
clowestab committed Feb 14, 2025
2 parents 8d4795e + 000980d commit e647420
Show file tree
Hide file tree
Showing 63 changed files with 1,919 additions and 227 deletions.
5 changes: 5 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
# default: alchemy,infura,ankr,drpc,public
PROVIDER_ORDER=

# provider keys
# note: priority order defined in test/providers.ts
ALCHEMY_KEY=
INFURA_KEY=
ANKR_KEY=
DRPC_KEY=

# true if non-empty
ALCHEMY_PREMIUM=

11 changes: 9 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ This repository provides an end-to-end solution for proving data from rollup cha

![Unruggable Gateways CI](https://github.com/unruggable-labs/unruggable-gateways/actions/workflows/unruggable-gateways.yml/badge.svg)

## Audits

The codebase has been audited. Details of our audits can be found [here](./audits/audits.md).

## Quickstart

`npm i @unruggable/gateways` [✓](https://www.npmjs.com/package/@unruggable/unruggable-gateways)
Expand Down Expand Up @@ -37,19 +41,22 @@ This repository provides an end-to-end solution for proving data from rollup cha
* [Nitro](./src/nitro/NitroRollup.ts)
* [Linea](./src/linea/LineaRollup.ts)
* [Polygon PoS](./src/polygon/PolygonPoSRollup.ts)
* [Polygon ZK](./src/polygon/ZKEVMRollup.ts) — *WIP*
* [Scroll](./src/scroll/ScrollRollup.ts)
* [Taiko](./src/taiko/TaikoRollup.ts)
* [ZKSync](./src/zksync/ZKSyncRollup.ts)
* [Reverse OP](./src/op/ReverseOPRollup.ts) — L2 → L1
* [Self](./src/eth/EthSelfRollup.ts) — any → itself
* [Trusted](./src/TrustedRollup.ts) — any → any
* [DoubleNitro](./src/nitro/DoubleNitroRollup.ts) — L1 → L2 → L3
* [Polygon ZK](./src/polygon/ZKEVMRollup.ts) — *WIP*
* [Morph](./src/morph/MorphRollup.ts) — *WIP*
* [Starknet](./src/starknet/StarknetRollup.ts) — *WIP*
* Provers
* [Eth](./src/eth//EthProver.ts) — `eth_getProof`
* [Linea](./src/linea/LineaProver.ts) — `linea_getProof`
* [ZKSync](./src/zksync/ZKSyncProver.ts) — `zks_getProof`
* [ZKEVM](./src/polygon/ZKEVMProver.ts) — `zkevm_getProof`
* [ZKEVM](./src/polygon/ZKEVMProver.ts) — `zkevm_getProof` — *WIP*
* [Starknet](./src/starknet/StarknetProver.ts) — `pathfinder_getProof` — *WIP*
* Verifier Hooks
* [Eth](./contracts/eth/EthVerifierHooks.sol) — [Patricia Merkle Tree](./contracts/eth/MerkleTrie.sol)
* [Linea](./contracts/linea/LineaVerifierHooks.sol) — [Sparse Merkle Tree](./contracts/linea/SparseMerkleProof.sol) + [Mimc](./contracts/linea/Mimc.sol)
Expand Down
54 changes: 54 additions & 0 deletions audits/audits.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# Audited Releases

| Audit Date | Release Link | NPM Package Link |
|------------|--------------|------------------|
| [December 2024 to February 2025](#december-2024-to-february-2025) | [v1.0.0](https://github.com/unruggable-labs/unruggable-gateways/releases/tag/v1.0.0) | [@unruggable/gateways@1.0.0](https://www.npmjs.com/package/@unruggable/gateways/v/1.0.0) |

# Audit Details

## December 2024 to February 2025

From **December 2024 to February 2025** the Unruggable Gateways codebase underwent a multipronged audit in collaboration with [CodeArena](https://code4rena.com/).

Please see [scoping.md](./supplementary/scoping.md) for information that was provided to CodeArena in advance of the audit.

### 1. Zenith Private Audit

Commit hash: [**v1.0.0-audit-2024-11-22-rc.1**](https://github.com/unruggable-labs/unruggable-gateways/releases/tag/v1.0.0-audit-2024-11-22-rc.1)

The Zenith audit was comprehensive and in-depth.

**Two medium** severity issues were found in our Scroll verifier as well as **five low** severity issues.

The [discovered issues](https://github.com/zenith-security/2024-11-unruggable-zenith/issues) were mitigated and an an audit report was produced by CodeArena.

Please see [Zenith Audit Report - Unruggable.pdf](./supplementary/Zenith%20Audit%20Report%20-%20Unruggable.pdf) for an indepth look at the findings.

### 2. Invitational Competitive Audit

Commit hash: [**v1.0.0-invitational-audit-2024-12-06-rc.2**](https://github.com/unruggable-labs/unruggable-gateways/releases/tag/v1.0.0-invitational-audit-2024-12-06-rc.2)

The invitational audit involved five auditors selected by the CodeArena team. The selected wardens were provided with the following [code walkthrough](https://www.youtube.com/watch?v=x4DG2iumwck), and had direct contact with the Unruggable team through Discord, and the CodeArena invitational platform.

Two medium severity issues were found:

- The Scroll verifier was not respecting the max depth of their zkTrie.
- The verifier for OP Stack chains implementing fault proofs did not correctly respect the blacklist.

Both issues were mitigated, and these mitigations were [reviewed](https://gist.github.com/peakbolt/959c3b286f104ba2038fa082ad3d5388).

A low severity issue was found pertaining to validations in the context of the **unfinalized** Arbitrum Nitro verifier. For strict correctness these additional validations were added but it is worth drawing the attention of users of this codebase to the fact that unfinalized verifiers are configured by users and their usage inherently involves relaxation of trust assumptions.

Changes implemented in response to the invitational audit can be found within the following [Pull Request](https://github.com/unruggable-labs/unruggable-gateways/pull/19).

### 3. Additional Zenith Review

Based on discussion with the CodeArena team and the judge for the invitational audit, it was decided that a member of the Zenth audit team (@peakbolt) would undertake a further coverage check to ensure that the codebase had received appropriate audit coverage.

This [additional review](https://gist.github.com/peakbolt/d09c5b3cd7e77d04af0d1a39755e715d) uncovered one further Medium risk issue:

- The OP stack verifier for chains **not implementing fault proofs** was not correctly considering `finalizationPeriodSeconds`.

This issue was mitigated using a GameFinder (binary search) approach to optimise finding appropriately finalised games from both the gateway and verifier code.

Additionally, two Low risk issues related to strict proof validation correctness were found. These issues were mitigated.
155 changes: 155 additions & 0 deletions audits/supplementary/Unruggable-MR.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
**NOTE:** The canonical source of this file is [here](https://gist.github.com/peakbolt/959c3b286f104ba2038fa082ad3d5388). This is included here for posterity.

---


# Fix for Finding F9
- https://code4rena.com/evaluate/2024-12-unruggable-invitational/findings/F-9

Confirmed issue is resolved with the added `MAX_TRIE_DEPTH (248)` check that ensures provided proof does not exceed the max depth of 248.


# Fix for Finding F10
- https://code4rena.com/evaluate/2024-12-unruggable-invitational/findings/F-10

The fix for F10 does not fully address the issue as `stakerCount` could also include zombies (stakers that lost in a challenge).

That is because when stakers lost a challenge, they are `turned into` zombies and not removed from stakerCount immediately. That means it is possible for rejected nodes to have `stakerCount > 0`.

In Nitro's code, the definition of no stakers refers to no zombie stakers. This is evident from [RollupUserLogic.sol](https://github.com/OffchainLabs/nitro-contracts/blob/v2.1.0/src/rollup/RollupUserLogic.sol#L108-L111) where the `rejectNextNode()` checks for zero non-zombies (active stakers) to proceed with the node rejection.

```solidity
function rejectNextNode(address stakerAddress) external onlyValidator whenNotPaused {
..
// Verify that no staker is staked on this node
require(
firstUnresolvedNode_.stakerCount == countStakedZombies(firstUnresolvedNodeNum),
"HAS_STAKERS"
);
```

## Recommendations
However, my recommendation is not to use `stakerCount` but instead rely on `firstUnresolvedNode()` and `latestConfirmedNode()` to skip rejected nodes in the following manner:

1. Trasverse backward from the `latestNodeCreated()` to find the node that satistify `_minBlocks`.
2. If the next iteration goes past the `firstUnresolvedNode()`, it should then continue the traversing using `latestConfirmedNode()` and traverse using `node.prevNum` to transfer through the confirmed nodes (just like finalized mode).

Doing so will allow us to skip the rejected nodes (below) in an efficient manner.

If we look at this [diagram](https://docs.arbitrum.io/assets/images/rollup-malicious-validator-1f29ab788995494d9d8fb6f617d6185d.svg) from Arbitrum we can see two scenarios of rejected nodes.

1. Rejected nodes (104, 105) that are after `latestConfirmedNode()` - It will skip these by jumping from `firstUnresolvedNode()` to `latestConfirmedNode()`, as it will skip the nodes that had been resolved and rejected by the protocol.

2. Rejected nodes (101) that are before `latestConfirmedNode()` - These are skipped simply by trasversing confirmed nodes like finalized mode, and not using descending node number. We need to consider this case as it is possible to select a node that is older than the `latestConfirmed()` node.


Note that unresolved node 111 will eventually be rejected by the protocol, as it is a child of rejected node 104. That means we could either treat it as unresolved and allow it to be selected by `getLatestContext()` or try to anticipate the rejection and not allow it to be selected.

My recommended solution treats node 111 as unresolved and allow it to be selected due to simplicity and respecting the protocol's state. But there is nothing wrong to reject node 111, though it is more complex as there are no direct way to check if the parent/ancestor node is rejected. Furthermore, there are also such pending rejection scenarios, such as deadline and rejection of child/decedent nodes, so its not easy to address them all.


### Client:
Fixed in https://github.com/unruggable-labs/unruggable-gateways/commit/daf1325791356512940511a2bdc54e28d32bba37

### Zenith:
Resolved with a new `_isNodeUsable()` that checks specified node is not rejected with the check `node.stakerCount > _rollup.countStakedZombies(index)`. When `node.stakerCount == _rollup.countStakedZombies(index)`, it means that the node has zero active stakers, which by definition is a rejected or pending rejection node.

# Fix for Finding F13
- https://code4rena.com/evaluate/2024-12-unruggable-invitational/findings/F-13

The fix addresses the reported issue by rejecting any DisputeGame that is blacklisted by the Guardian when the game is resolved incorrectly.

However, the fix fails to account for the `DISPUTE_GAME_FINALITY_DELAY_SECONDS` in [OptimismPortal2.sol#L67](https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L67). This is known as the [Air-gap period](https://specs.optimism.io/fault-proof/stage-one/bridge-integration.html#air-gap), where a game is only considered finalized when it is resolved for at least `DISPUTE_GAME_FINALITY_DELAY_SECONDS`. The purpose of it is to provide sufficient time for the Guardian to blacklist any incorrectly resolved games.

Furthermore, this air-gap period check extends beyond withdrawal finalization as it is used in [AnchorStateRegisty#L203-L248](https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/dispute/AnchorStateRegistry.sol#L203-L248), to determine if the game claim is finalized in a generic manner.

```Solidity
/// @notice Returns whether a game is finalized.
/// @param _game The game to check.
/// @return Whether the game is finalized.
function isGameFinalized(IDisputeGame _game) public view returns (bool) {
// Game must be resolved.
if (!isGameResolved(_game)) {
return false;
}
// Game must be beyond the airgap period.
if (!isGameAirgapped(_game)) {
return false;
}
return true;
}
/// @notice Returns whether a game's root claim is valid.
/// @param _game The game to check.
/// @return Whether the game's root claim is valid.
function isGameClaimValid(IDisputeGame _game) public view returns (bool) {
// Game must be a proper game.
bool properGame = isGameProper(_game);
if (!properGame) {
return false;
}
// Must be respected.
bool respected = isGameRespected(_game);
if (!respected) {
return false;
}
// Game must be finalized.
bool finalized = isGameFinalized(_game);
if (!finalized) {
return false;
}
// Game must be resolved in favor of the defender.
if (_game.status() != GameStatus.DEFENDER_WINS) {
return false;
}
return true;
}
```

## Recommendations (Previous)
It is recommended to use [`AnchorStateRegistry.isGameClaimValid()`](https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/dispute/AnchorStateRegistry.sol#L203-L248) for finalized mode and also adapt it for unfinalized mode (to check game is proper, respected and not rejected). This will ensure that the verifier mirrors the OP's game verification.


## Recommendations (Latest as of 31 Jan 2025)

As the previous recommendations above are based on new contracts that are not yet deployed, it will be better to mirror the current OP's game verification based on their withdrawal for the deployed [OptimismPortal](https://github.com/ethereum-optimism/optimism/blob/v1.8.0/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L505-L518).

Specifically, the new recommendation is to add the following checks for finalized mode (`minAgeSec == 0`),
- Check game was not created before `portal.respectedGameTypeUpdatedAt`
- Ensure that the game has been resolved for at least `DISPUTE_GAME_FINALITY_DELAY_SECONDS` (passed airgapped period)

These checks currently exists in `checkWithdrawal()` in [OptimismPortal2.sol#L505-L518](https://github.com/ethereum-optimism/optimism/blob/v1.8.0/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L505-L518)
```solidity
// The game must have been created after `respectedGameTypeUpdatedAt`. This is to prevent users from creating
// invalid disputes against a deployed game type while the off-chain challenge agents are not watching.
require(
createdAt >= respectedGameTypeUpdatedAt,
"OptimismPortal: dispute game created before respected game type was updated"
);
// Before a withdrawal can be finalized, the dispute game it was proven against must have been
// resolved for at least `DISPUTE_GAME_FINALITY_DELAY_SECONDS`. This is to allow for manual
// intervention in the event that a dispute game is resolved incorrectly.
require(
block.timestamp - disputeGameProxy.resolvedAt().raw() > DISPUTE_GAME_FINALITY_DELAY_SECONDS,
"OptimismPortal: output proposal in air-gap"
);
```

### Client:
Fixed in https://github.com/unruggable-labs/unruggable-gateways/commit/92c9fbb36dee3a11e0cff1096ce4958ac1491ae6

### Zenith:
Resolved by adding finalized mode validation to check that the selected game is finalized as follows,
```
(created > respectedGameTypeUpdatedAt &&
status == DEFENDER_WINS &&
block.timestamp - gameProxy.resolvedAt()) > DISPUTE_GAME_FINALITY_DELAY_SECONDS)
```

Note that in finalized mode, only games with respected game type is considered valid (`gameTypeBitMask == 0`). Also, it only consider games with `createdAt > respectedGameTypeUpdatedAt` as games with `createdAt <= respectedGameTypeUpdatedAt` are considered retired in OP now.
Binary file not shown.
78 changes: 78 additions & 0 deletions audits/supplementary/scoping.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
# Introduction

Unruggable Gateways implements a complete solution for fetching proofs of data from rollup chains and verifying that data on Layer 1 Ethereum.

These gateways will be utilised as a middleware piece for fetching proven data from rollup chains. Initially these gateways will be operated in the context of cross chain reverse resolution. Later they will be used for forward resolution of ENS names as part of the [ENS v2](https://blog.ens.domains/post/ensv2) roadmap.

The scope of this security audit is the Virtual Machine (`GatewayVM.sol`) that interprets requests sent to our HTTP gateways, and the verifier contracts (that will be deployed on Layer 1 Ethereum) that verify proofs returned from the aforementioned gateway server in response to a ERC-3668 CCIP requests.

The initial release will support Arbitrum, Base, Linea, Optimism, and Scroll so the scope will be constrained to the verifiers for those chains.

## 1. Repository Links

- [https://github.com/unruggable-labs/unruggable-gateways](https://github.com/unruggable-labs/unruggable-gateways/)

## 2. Branches

- unruggable-gateways: **main**

## 3. Contract Design

- The `*Verifier.sol` contracts are called from the ERC3668 callback function and verify the proof data returned from the gateway giving consideration to chain specific rollup architecture.
- `*Verifier.sol` contracts inherit from `AbstractVerifier.sol` and implement the `IGatewayVerifier.sol` interface.
- The `*VerifierHooks.sol` contain the logic for the chain specific verification of both account state and storage state.
- `*VerifierHooks.sol` contracts implement the `IVerifierHooks.sol` interface.

## File paths to INCLUDE

**unruggable-gateways**

```
[SLOC] FILE_NAME
[45] contracts/eth/EthVerifierHooks.sol
[296] contracts/eth/MerkleTrie.sol # op code, ENS picked older commit
[65] contracts/eth/SecureMerkleTrie.sol # op code
[14] contracts/linea/ILineaRollup.sol
[40] contracts/linea/LineaVerifier.sol
[109] contracts/linea/LineaVerifierHooks.sol
[878] contracts/linea/Mimc.sol # linea code, only used for local
[267] contracts/linea/SparseMerkleProof.sol # linea code, only used for local
[24] contracts/nitro/IRollupCore.sol
[91] contracts/nitro/NitroVerifier.sol
[106] contracts/op/OPFaultGameFinder.sol
[111] contracts/op/OPFaultVerifier.sol
[72] contracts/op/OPVerifier.sol
[45] contracts/scroll/ScrollVerifier.sol
[200] contracts/scroll/ScrollVerifierHooks.sol # this was written to be debugged
[37] contracts/AbstractVerifier.sol
[68] contracts/GatewayFetchTarget.sol
[481] contracts/GatewayFetcher.sol
[89] contracts/GatewayRequest.sol # just defs: constants + struct
[515] contracts/GatewayVM.sol
[9] contracts/IGatewayProtocol.sol
[14] contracts/IGatewayVerifier.sol
[18] contracts/IVerifierHooks.sol
[30] contracts/RLPReaderExt.sol
[12] contracts/ReadBytesAt.sol
[3636 inc supplementary files/2130]
Source code lines discerned using: https://ghloc.vercel.app/unruggable-labs/unruggable-gateways?branch=main&locsPath=%5B%22contracts%22%5D
```

## Priority files

The following files should receive extra attention:

- `contracts/GatewayVM.sol` is the virtual machine implementation that evaluates requests sent to the Gateway.

## Areas of concern

- Are there any oversights in the verifier contracts that allow for verification of an invalid proof and thus return of invalid data?
- Noting the inherent complexities of proof verification does the architecture of the solution appropriately balance code readability/extensibility/flexibility/optimisations?
Loading

0 comments on commit e647420

Please sign in to comment.