Add additional verification to EspressoTEEVerifier #28

Sneh1999 · 2024-11-25T22:38:52Z

Closes #312

I have listed down my findings related to automata here

This PR:

Adds additional verification checks to the EspressoTEEVerifier.sol
Cleans up and adds doc strings to deprecated functions
Adds some tests to test our verification logic

Important Files to Review

src/bridge/EspressoTEEVerifier.sol
src/bridge/SequencerInbox.sol

Additional Verifications Added

We check that the MR_ENCLAVE and MR_SIGNER match the expected values
We check that the hash of the function arguments sent by the batch poster is what was signed by the TEE
(NOTE: we dont need to check the signature because automata already verifies that, we only check that the reportData matches the function arguments)

Sneh1999 · 2024-11-26T19:55:07Z

src/bridge/EspressoTEEVerifier.sol

+        @param rawQuote The raw quote in bytes
+        @return header The parsed header
+    */
+    function parseQuoteHeader(bytes calldata rawQuote) public pure returns (Header memory header) {


I didnt see any harm in making it public and I think it might turn out helpful for debugging in the future. Its a pure function so takes very less gas as well.

Sneh1999 · 2024-11-26T19:57:18Z

test/foundry/EspressoTEEVerifier.t.sol

-        qeIdDaoUpsert(3, qeIdPath);
-        fmspcTcbDaoUpsert(tcbInfoPath);
+    function setUp() public {
+        vm.createSelectFork("https://rpc.ankr.com/eth_sepolia");


I am forking sepolia in tests here instead of arb sepolia because automata hasnt deployed there contracts on arb sepolia yet. I will migrate these tests once they have done that

Sneh1999 · 2024-11-26T20:00:08Z

src/bridge/EspressoTEEVerifier.sol


-    /**
+    constructor(bytes32 _mrEnclave, bytes32 _mrSigner, address _quoteVerifier) {


The reason I am accepting the V3QuoteVerifier contract here is because automata changed how they manage their contracts. Now each Verifier contract should be registered with the PCCSRouter

sveitser · 2024-11-27T08:36:12Z

src/bridge/EspressoTEEVerifier.sol

+    V3QuoteVerifier
+} from "@automata-network/dcap-attestation/contracts/verifiers/V3QuoteVerifier.sol";
+import {BytesUtils} from "@automata-network/dcap-attestation/contracts/utils/BytesUtils.sol";
+import {Ownable} from "solady/auth/Ownable.sol";


Is there a particular reason we're not using the openzeppelin Ownable here?

Automata uses this library for ownable. Apparently is more gas efficient

sveitser · 2024-11-27T08:45:29Z

test/foundry/EspressoTEEVerifier.t.sol

+        string memory inputFile = string.concat(vm.projectRoot(), quotePath);
+        bytes memory sampleQuote = vm.readFileBinary(inputFile);
+        espressoTEEVerifier = new EspressoTEEVerifier(
+            bytes32(0x51dfe95acffa8a4075b716257c836895af9202a5fd56c8c2208dacb79c659ff1),


Define a variable like badMrEnclave here? And similarly for the other tests. Makes it a bit easier to read especially since the value is the same except for the last digit.

To be more certain that we are always reverting for the right reasons I think it would be a bit better to have custom Error types for each reason we fail verification and check in the tests if we are reverting for the right reasons. Suggestion for later cleanup work.

I will clean this up, might as well do it here!

test/foundry/SequencerInboxTEE.t.sol

test/foundry/EspressoTEEVerifier.t.sol

test/foundry/SequencerInboxTEE.t.sol

sveitser · 2024-11-27T09:25:46Z

What's the best way to run the tests locally? Both yarn build:all (hardhat compile) and forge build seem to take ages.

sveitser · 2024-11-27T09:30:49Z

I also get an error from forge build

[⠃] Compiling...2024-11-27T09:30:26.588485Z ERROR foundry_compilers_artifacts_solc::sources: error="/home/lulu/r/EspressoSystems/nitro-espresso-integration/contracts/node_modules/@openzeppelin/contracts/utils/Pausable.sol": No such file or directory (os error 2)
[⠒] Compiling...
[⠒] Compiling 275 files with Solc 0.8.25^C⏎

That contract seems to be at @openzeppelin/contracts/security/Pausable.sol instead

sveitser

Looks fine to me. Some nitpicks and I haven't been able to run it locally because the compilation takes seemingly forever.

Sneh1999 · 2024-11-27T13:26:29Z

What's the best way to run the tests locally? Both yarn build:all (hardhat compile) and forge build seem to take ages.

I did do forge build - yes it takes around 10-15 minutes

Sneh1999 · 2024-11-27T18:12:36Z

src/bridge/EspressoTEEVerifier.sol

@@ -136,13 +146,6 @@ contract EspressoTEEVerifier is Ownable {
        success = true;
    }

-    /*


we dont need this as we can call transferOwnership function present in the Ownable contract

* Add additional verification to EspressoTEEVerifier * fix tests * fix tests * cleanup * revert broken tests * fix ci * fix ci * address comments * address comments * add code docs

Sneh1999 added 3 commits November 25, 2024 17:35

Add additional verification to EspressoTEEVerifier

62dbad6

fix tests

29d1104

fix tests

13fe218

Sneh1999 commented Nov 26, 2024

View reviewed changes

Sneh1999 added 4 commits November 26, 2024 15:06

cleanup

c710201

revert broken tests

7d25c91

fix ci

7327a3d

fix ci

7c3d1bd

Sneh1999 marked this pull request as ready for review November 26, 2024 23:02

Sneh1999 requested review from alxiong, sveitser, alysiahuggins, jbearer, ImJeremyHe and zacshowa November 26, 2024 23:02

sveitser reviewed Nov 27, 2024

View reviewed changes

test/foundry/SequencerInboxTEE.t.sol Show resolved Hide resolved

sveitser reviewed Nov 27, 2024

View reviewed changes

test/foundry/EspressoTEEVerifier.t.sol Outdated Show resolved Hide resolved

sveitser reviewed Nov 27, 2024

View reviewed changes

test/foundry/SequencerInboxTEE.t.sol Outdated Show resolved Hide resolved

sveitser reviewed Nov 27, 2024

View reviewed changes

test/foundry/SequencerInboxTEE.t.sol Outdated Show resolved Hide resolved

sveitser self-requested a review November 27, 2024 09:30

sveitser approved these changes Nov 27, 2024

View reviewed changes

Sneh1999 commented Nov 27, 2024

View reviewed changes

address comments

cc05e5d

Sneh1999 force-pushed the add-verification branch from 64c3051 to cc05e5d Compare November 27, 2024 18:46

address comments

c308e83

add code docs

701f70d

Sneh1999 merged commit f8a152f into celestia-integration Nov 28, 2024
7 of 8 checks passed

Sneh1999 deleted the add-verification branch November 28, 2024 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional verification to EspressoTEEVerifier #28

Add additional verification to EspressoTEEVerifier #28

Sneh1999 commented Nov 25, 2024 •

edited

Loading

Sneh1999 Nov 26, 2024

Sneh1999 Nov 26, 2024

Sneh1999 Nov 26, 2024

sveitser Nov 27, 2024

Sneh1999 Nov 27, 2024

sveitser Nov 27, 2024

Sneh1999 Nov 27, 2024

Sneh1999 Nov 27, 2024

sveitser commented Nov 27, 2024

sveitser commented Nov 27, 2024 •

edited

Loading

sveitser left a comment

Sneh1999 commented Nov 27, 2024

Sneh1999 Nov 27, 2024


		/**
		constructor(bytes32 _mrEnclave, bytes32 _mrSigner, address _quoteVerifier) {

Add additional verification to EspressoTEEVerifier #28

Add additional verification to EspressoTEEVerifier #28

Conversation

Sneh1999 commented Nov 25, 2024 • edited Loading

This PR:

Important Files to Review

Additional Verifications Added

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sveitser commented Nov 27, 2024

sveitser commented Nov 27, 2024 • edited Loading

sveitser left a comment

Choose a reason for hiding this comment

Sneh1999 commented Nov 27, 2024

Choose a reason for hiding this comment

Sneh1999 commented Nov 25, 2024 •

edited

Loading

sveitser commented Nov 27, 2024 •

edited

Loading