Moves workloadId to builder policy, adjusts registry to accept extended registration data #21

Ruteri · 2025-07-09T16:19:27Z

Working out some ideas in practice. I'll suggest changes to the spec to match before merging.

…ed user data

Melvillian

See inline comments.

Like you said, this is a draft and still needs some more work on it.

Here's my understanding of this PR:

The overall goal of this is to make flashtestations more flexible, by allowing the user to both define their own definition of “valid” by encoding the WorkloadId however they wish, and by extending the amount of data that can be attested to by the TEE beyond just the 64 bytes of reportData

Moving Workload Id out of registry and into blockbuilding, which allows the definition of workloadID to be policy-specific.
adding appData which allows arbitrary data to be attested to by the TEE via a 32-byte app data hash embeded in the reportData
adding ValidateTDConfig to do some additional security checks on those MR values

Here's some remaining questions I have:

What other changes do you want to make? We have 2 weeks until code freeze, and these changes are going to take 1-2 weeks to get updated and then to have Sarah + Ferran verify them.
Why was ValidateTDConfig added? What do these xFAM and tdAttribute mask checks get us that we didn’t have before?
Are the public keys understood to be in the appData?

Melvillian · 2025-07-14T11:26:02Z

README.md

+   a. Policy contract checks signature against registry of registered TEEs

-   b. emit an event indicating the block was built by a particular TEE device (identified by its WorkloadId)
+   b. Policy derives WorkloadId from the stored report body


readability: it's not clear why this step is important for "Verify Flashtestation Transaction", can you extend this step to explain how the WorkloadId gets used?

src/BlockBuilderPolicy.sol

Melvillian · 2025-07-14T12:00:38Z

src/FlashtestationRegistry.sol

+     * @param appData The extended attested to data
     */
-    function registerTEEService(bytes calldata rawQuote) external limitBytesSize(rawQuote) nonReentrant {
+    function registerTEEService(bytes calldata rawQuote, bytes calldata appData)


readability: this comment should explain why and how we use the app data hash to bind the TEE-attested reportData to the appData. Otherwise it's confusing why this data isn't just included in the rawQuote and why the cryptographi binding is necessary.

src/BlockBuilderPolicy.sol

src/FlashtestationRegistry.sol

src/interfaces/IFlashtestationRegistry.sol

src/BlockBuilderPolicy.sol

Melvillian

Needs tests to be fixed and additional tests for the new functionality

extendedRegistratioData verification
BlockBuilderPolicy isAllowedPolicy checks
TDConfigParams-related tests

See inline comments as well.

I'm not OK with the centralized xfam + tdAttributes changes as is because I think makes liveness depend too much on an honest centralized owner. See my suggested fix, and if you don't think that'll work I think we remove it.

src/interfaces/IFlashtestationRegistry.sol

README.md

src/FlashtestationRegistry.sol

Melvillian · 2025-07-16T17:41:35Z

src/interfaces/IFlashtestationRegistry.sol

    // Events
-    event TEEServiceRegistered(
-        address teeAddress, WorkloadId workloadId, bytes rawQuote, bytes publicKey, bool alreadyExists
+    event TEEServiceRegistered( /* dev: could be hash of the quote */


remove, it's no longer needed

Hm, are you sure? I'd imagine the event itself is still helpful. I don't mind removing it if you think it's not helping.

haha wait sorry definitely keep the event, I just meant the comment (sorry, should have been more specific)

src/FlashtestationRegistry.sol

Melvillian · 2025-07-16T18:38:29Z

src/FlashtestationRegistry.sol

-        bytes memory rawQuote = registeredTEEs[teeAddress].rawQuote;
-        require(rawQuote.length > 0, TEEServiceNotRegistered(teeAddress));
-        return QuoteParser.parseV4Quote(rawQuote);
+    function setTDConfigParams(bytes8 xfamMask, bytes8 tdAttributesMask) external onlyOwner {


I don't like how this is an onlyOwner function, which means it is trivial to affect the liveness of the FlashtestationsRegistry; the owner sets the masks to 0xFFFFFFFFFFFFFFFF and then getRegistration will always fail because validateTdConfig always fails.

But, we need this here so that we can prevent attesting to reports with insecure xfam and tdAttribute values.

What if we refactor this function so that in registerTEEService the user passes in their desired xfamMask and tdAttributesMask, and we set that in the RegisteredTEE struct. However, if they pass in a 0x0 for either of xfam or tdAttributes, then we set our default tdConfigParams, which is only set by onlyOwner. Now there is no way for the owner to unilaterally shut down the protocol by updating the TCConfigParams to a malicious value.

When a TCB level becomes outdated or insecure, then the user can call registerTEEService again with a different quote, and the RegisteredTEE will be securely updated.

tl;dr: I think that making the liveness of FlashtestationsRegistry dependent on a centralized owner acting honestly is too much centralization, and we need to find another way around it. Yes, the owner has god-powers by being able to upgrade the contract, but the goal is for that to go away once there's product-market fit and bugs have been squashed. Allowing this liveness centralization is a problem that we should not introduce at the beginning unless we have a clear path for removing it later on (which we don't).

Agreed on the registry owner not being a good candidate for owning the masks, good callout.
I think passing the masks to validation would work, however something even simpler could be masking out the bits when creating the workload id — I think it's the cleanest way to do it, see if you like it too.

src/FlashtestationRegistry.sol

Melvillian

See my inline comments, but I'm approving so that we can merge this.

Melvillian · 2025-07-18T18:33:07Z

script/Interactions.s.sol

        // the outputs in future scripts (like Interactions.s.sol:AddWorkloadToPolicyScript)
        address sender = vm.getWallets()[0];
-        (WorkloadId workloadId,,, bytes memory publicKey) = registry.registeredTEEs(sender);
+        (,, bytes32 quoteHash,,) = registry.registeredTEEs(sender);


remove, this isn't stored anymore in registeredTEEs

src/BlockBuilderPolicy.sol

Melvillian · 2025-07-18T18:51:17Z

src/BlockBuilderPolicy.sol

    /// @param blockContentHash The hash of the block content
    /// @dev This function is internal because it is only used by the permitVerifyBlockBuilderProof function
    /// and it is not needed to be called by other contracts
+    /// @dev We can't check the whole block content hash, but we could check the block number and parent hash


readability: you're right, but it feels strange to keep this comment here, because it feels more like a comment you'd leave in a PR review, rather than in the code itself.

For instance, imagine a user unfamiliar with the code reading this, the natural response to this question is "why didn't the creators check the block number and parent hash? Is something wrong this this implementation?"

I think we should remove it to reduce confusion.

Melvillian · 2025-07-18T19:30:32Z

src/FlashtestationRegistry.sol

-        emit TEEServiceRegistered(teeAddress, workloadId, rawQuote, publicKey, previouslyRegistered);
+    function registerTEEService(bytes calldata rawQuote, bytes calldata extendedRegistrationData)
+        external
+        limitBytesSize(rawQuote)


instead of doing limitBytesSize here, let's just add it to doRegister so it is called for both registerTEEService and permitRegisterTEEService.

We missed adding the limitBytesSize(extendedRegistrationData) here, so let's protect against this problem by locating the logic in one place.

Melvillian · 2025-07-18T20:44:34Z

src/FlashtestationRegistry.sol

+        bytes calldata extendedRegistrationData,
+        uint256 nonce,
+        bytes calldata signature
+    ) external limitBytesSize(rawQuote) limitBytesSize(extendedRegistrationData) nonReentrant {


see my comment above about limitBytesSize

Melvillian · 2025-07-18T21:05:28Z

src/interfaces/IFlashtestationRegistry.sol

-        bytes publicKey; // The 64-byte uncompressed public key of TEE-controlled address, used to encrypt messages to the TEE
+        bytes rawQuote; // The raw quote from the TEE device, which is stored to allow for future quote quote invalidation
+        TD10ReportBody parsedReportBody; // Parsed form of the quote to avoid unnecessary parsing
+        bytes extendedRegistrationData; // Any additional attested data for application purposes


We don't ever explain or give examples of what extendedRegistrationData could be, so this seems like a good place to do that.

What do we foresee this being used for? Some examples I can see are including the publicKey of the teeAddress that registered the TEE, including the operatorId` for extra security against phyical TEE attacks.

Please add a comment explaining this. Something like:

// Any additional attested data for application purposes. This data is attested by a bytes32 in the attestation's reportData, where that bytes32 is a hash of the ABI-encoded values in the extendedRegistrationData all concatenated together. This registration data can be anything that's needed for your app, for instance an operatorIdPublicKey that allows you to verify signatures from your TEE operator.

Melvillian

See my inline comments, but besides those there is one final group of tests I'd like you to add; tests for the xfam and tdAttributes bitmasking logic. In particular, the tests should validate the following 2 invariants (these come from the BlockBuilderPolicy.sol comments:

// We expect fpu and sse xfam bits to be set, and anything else should be handled by explicitly allowing the workloadid
        bytes8 xfamExpectedBits = (fullBitmask ^ (TD_XFAM_FPU | TD_XFAM_SSE));

        // We don't mind ve disabled, pks, and kl tdattributes bits being set either way, anything else requires explicitly allowing the workloadid
        bytes8 tdAttributesBitmask = (fullBitmask ^ (TD_TDATTRS_VE_DISABLED | TD_TDATTRS_PKS | TD_TDATTRS_KL));

To test these, you can read in the quote data from one of the raw_tdx_quotes, tweak the necessary bits of the parsedReportBody that correspond to parsedReportBody.xFAM and parsedReportBodytdAttributes, and verify that all of the permutations of xfam/tdAttributes bit values uphold the 2 invariants above.

Oh also, see this:

this doc link still needs to be updated: https://github.com/flashbots/flashtestations/pull/21/files#r2216670103. You marked it as resolved but I don't think it's resolved.

Melvillian · 2025-07-23T16:23:29Z

test/FlashtestationRegistry.t.sol

+    }
+
+    function test_extDataMismatch() public {
+        // This test verifies that the TEE address is properly extracted from reportData[0:20]


this comment is copypasta from the test above, it should say

`// This test verifies that using a mismatched extendedRegistrationData will result in an error

Melvillian · 2025-07-23T16:24:40Z

test/FlashtestationRegistry.t.sol

+
+        attestationContract.setQuoteResult(mockQuote, true, mockOutput);
+
+        // Can only register from the address in the quote


same thing here, should say

`// keccak hash of extendedRegistrationData must match the 32 byte hash in the reportData

Melvillian · 2025-07-23T16:32:59Z

src/BlockBuilderPolicy.sol

+        // We expect fpu and sse xfam bits to be set, and anything else should be handled by explicitly allowing the workloadid
+        bytes8 xfamExpectedBits = (fullBitmask ^ (TD_XFAM_FPU | TD_XFAM_SSE));
+
+        // We don't mind ve disabled and pks tdattributes bits being set either way, anything else requires explicitly allowing the workloadid


your comment only includes 2 of the attributes, but the code line below includes 3 (VE_DISABLED, PKS, and KL). Should the comment be updated or the code? Either way, please make the code and comment align, and be correct.

Ruteri added 7 commits July 9, 2025 18:14

Moves workloadId to builder policy, adjusts registry to accept extend…

83337c9

…ed user data

retab

9374e07

Updates README, renames to appData

7550a6e

Formats

5f57947

Caches workload id, slightly refactors functions

3edb6ca

Minor changes

ba4bb15

Fixes src/ compilation issues

17cdb66

Melvillian reviewed Jul 14, 2025

View reviewed changes

Ruteri added 3 commits July 14, 2025 15:19

Separate registration data

cb9e16d

Simplifies changes

8ad154c

Updates README and makes src compile

e71010b

Melvillian requested changes Jul 16, 2025

View reviewed changes

Moves xfam and tdattrs back to workloadid

70c6bf8

Melvillian approved these changes Jul 18, 2025

View reviewed changes

Ruteri added 2 commits July 21, 2025 15:09

Adjusts compilation issues in test

9041cdb

Updates test, test data, adds a mock quote mgmt scripts

671d1ce

Melvillian marked this pull request as ready for review July 22, 2025 18:30

Melvillian mentioned this pull request Jul 22, 2025

Add commit hash to policy #22

Merged

Ruteri changed the title ~~[not canonical] Moves workloadId to builder policy, adjusts registry to accept extended user data~~ Moves workloadId to builder policy, adjusts registry to accept extended registration data Jul 23, 2025

Fixes warnings, applies review suggestions

acd240c

Melvillian requested changes Jul 23, 2025

View reviewed changes

Simplifies bitmasks, adds tests

2412eab

Melvillian approved these changes Jul 23, 2025

View reviewed changes

Melvillian merged commit c304f5c into flashbots:main Jul 23, 2025
1 check passed


		attestationContract.setQuoteResult(mockQuote, true, mockOutput);

		// Can only register from the address in the quote

Moves workloadId to builder policy, adjusts registry to accept extended registration data #21

Moves workloadId to builder policy, adjusts registry to accept extended registration data #21

Uh oh!

Conversation

Ruteri commented Jul 9, 2025

Uh oh!

Melvillian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Melvillian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ruteri Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Melvillian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Melvillian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Ruteri Jul 18, 2025 •

edited

Loading