Conversation
Closed
h4x3rotab
commented
Aug 4, 2025
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
h4x3rotab
force-pushed
the
foundry
branch
6 times, most recently
from
5f944b5 to
175836d
Compare
Migrates the KMS authorization smart contracts and bootAuth server from
Hardhat to Foundry. The Solidity sources are functionally identical to
master (pragma bumped ^0.8.22 → ^0.8.24 for OpenZeppelin 5.x, forge fmt
applied); the ABI, events, and storage layout are byte-compatible with
the live UUPS proxies on Phala mainnet.
Stack changes:
- Hardhat dependencies and config removed (hardhat.config.ts, typechain
types, jest.integration config, all hardhat-bound .test.ts files,
scripts/{deploy,upgrade,verify}.ts).
- Foundry stack added: foundry.toml, three lib/ submodules pinned at
forge-std v1.9.7, openzeppelin-contracts-upgradeable v5.4.0, and
openzeppelin-foundry-upgrades v0.4.0; a Foundry .t.sol test suite
(46 unit tests covering TCB toggle, factory deploy, upgrade paths,
and storage compatibility from legacy 5-arg initializers); production
deployment / management / query / upgrade scripts under script/.
- BootAuth Fastify server retained byte-identical except src/ethereum.ts,
which swaps typechain for a 4-method hand-written ABI (same struct,
same selectors, functionally identical).
- .openzeppelin/unknown-2035.json (proxy registry for the four live
Phala-mainnet proxies) restored for historical reference.
Operator-script fixes surfaced during a post-rebase audit:
- script/Upgrade.s.sol previously only had UpgradeKmsToV2 /
UpgradeAppToV2 pointing at test-only mock contracts. Added UpgradeKms
/ UpgradeApp scripts that upgrade live proxies to the current
production source.
- script/Manage.s.sol::DeployApp was calling the legacy 5-arg
deployAndRegisterApp, silently forcing requireTcbUpToDate=false. Now
reads REQUIRE_TCB_UP_TO_DATE env var and uses the 6-arg overload to
match master's hardhat-task semantics.
Security hardening:
- Both contracts switched from OwnableUpgradeable to
Ownable2StepUpgradeable. ERC-7201 namespaced storage means no slot
collision on upgrade; transferOwnership now stages a pending owner
who must acceptOwnership, eliminating the typo-bricks-contract risk.
- registerApp's permissionless-by-design intent documented inline in
natspec (any non-zero address can be registered by anyone; the
downstream allowedOsImages whitelist + delegated isAppAllowed gate
authorization).
- Slither static analysis configured in slither.config.json with
per-line suppression comments + justifications on the four
noise-detector hits (factory reentrancy-benign, unused-return on the
named-return forward pattern, two unindexed-event-address for
backward-compatible log indexers). Baseline: 0 findings.
- Inherited Prek hooks (trailing-whitespace, end-of-file-fixer,
shellcheck) cleaned up across the anvil helper scripts that came in
with the original migration.
Verification: forge fmt --check, forge build, forge test --ffi (46/46),
slither (0 findings), npx jest (4/4 server tests), npx tsc --noEmit
all clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Migrates
kms/auth-eth/from Hardhat to Foundry. Contract logic is unchanged from master; the only contract-source changes are:^0.8.22 → ^0.8.24(OpenZeppelin 5.x) +forge fmtOwnableUpgradeable → Ownable2StepUpgradeableon bothDstackKmsandDstackApp— a real authorization change (ownership transfer becomes two-step:transferOwnershipstages a pending owner who must callacceptOwnership). This eliminates the typo-bricks-contract risk of single-step transfer.ABI, events, and sequential storage layout are byte-identical to master (verified with
forge inspect … storageLayout). Ownable2Step adds its_pendingOwnerin a separate ERC-7201 namespaced slot, so it does not shift any sequential slot — an existing single-step proxy can be upgraded in place.Compatibility — verified against live Base deployment
The live KMS proxy (
0x2f83…Ba9C) and DstackApp proxy (0xc951…c2a2) on Base were checked directly:appImplementation); a mainnet-fork upgrade rehearsal to this PR's implementation preservedowner/gatewayAppId/appImplementation/all sequential slots, and the new two-step ownership flow works end-to-end._upgradesDisabled = true— it is permanently frozen and out of scope for any upgrade.Beyond the migration
UpgradeKms/UpgradeApp(production source) toscript/Upgrade.s.sol; the…ToV2variants target test-only mocks and are labeled do-not-run-against-prod.script/Manage.s.sol::DeployAppreadsREQUIRE_TCB_UP_TO_DATEand uses the 6-argdeployAndRegisterApp.src/ethereum.tsswaps typechain for a hand-written ABI; theAppBootInfotuple matches the Solidity struct field-for-field.Related PR
Formal verification (Slither + Halmos + spec) is isolated to #689, based on this branch.
Test plan
forge fmt --check,forge build,forge test --ffi→ 46/46npx jest(server) → 4/4Note for reviewers / operators
transferOwnershipon the upgraded KMS must follow withacceptOwnership(two-step now).UpgradeKmsToV2/UpgradeAppToV2mock scripts against production proxies.🤖 Generated with Claude Code