fix(tholos-v2): pin admin in __constructor to close initialize front-running - #183
Conversation
collinsezedike
left a comment
There was a problem hiding this comment.
The code change is correct, but two doc files this PR doesn't touch go stale and will actively mislead readers. docs/src/DEPLOYMENT_V2.md's deploy example (around line 65) still shows the old two-step pattern, deploy with no constructor args, then invoke initialize --admin, which will now fail outright since initialize no longer accepts admin at all. docs/src/CONTRACT_V2.md still documents initialize's old signature and admin semantics (around line 211) with no entry at all for the new __constructor entrypoint, so an integrator builds against the wrong ABI or can't figure out why their old initialize(admin, ...) call fails. Please update both.
|
Reopening to retrigger CI, no checks were ever registered on this PR. |
|
This PR now has a merge conflict with main and has never had a CI run registered on it (closing/reopening didn't trigger one either). Please rebase onto main, resolve the conflict, and push. That should both fix the conflict and give CI a fresh commit to actually run against. |
fbb36fc to
03e8664
Compare
|
@collinsezedike Changes are up, ready for your review again.
|
collinsezedike
left a comment
There was a problem hiding this comment.
One thing inline, plus one outside this diff: set_admin's doc comment (contracts/tholos-v2/src/lib.rs:793) and the matching CONTRACT_V2.md set_admin entry still say it fails with NotInitialized before initialize. That's no longer true now that Admin is set atomically in __constructor rather than in initialize, calling set_admin right after deploy but before initialize now succeeds instead of failing as documented. Please update both.
| env.storage().instance().set(&DataKey::Admin, &admin); | ||
| env.storage() | ||
| .instance() | ||
| .extend_ttl(INSTANCE_LIFETIME_THRESHOLD, INSTANCE_BUMP_AMOUNT); |
There was a problem hiding this comment.
This duplicates touch_instance_ttl's logic verbatim (env.storage().instance().extend_ttl(INSTANCE_LIFETIME_THRESHOLD, INSTANCE_BUMP_AMOUNT)) instead of calling Self::touch_instance_ttl(&env), which every other entrypoint in this contract already uses for exactly this. Not a bug today since both use the same constants, but a future change to the TTL bump logic that only updates touch_instance_ttl would silently leave this copy stale.
collinsezedike
left a comment
There was a problem hiding this comment.
Thanks for this, both prior findings are correctly fixed. Merging now.
Summary
Closes #154.
initializeincontracts/tholos-v2tookadminas a caller-suppliedparameter and only guarded against a second call (
AlreadyInitialized).Since deploy and initialize are separate Soroban transactions, nothing tied
the call to whoever actually deployed the instance — a party watching the
mempool could submit their own
initializecall with their ownadminaddress first and become the permanent admin of a contract someone else
paid to deploy.
Fixed by moving
admininto a__constructor(env, admin), which the hostinvokes atomically as part of the same
CreateContractV2operation thatcreates the instance — no transaction can execute in between "this instance
exists" and "its admin is recorded".
initializeno longer accepts anadminparameter at all: it reads the admin already fixed by__constructorfrom storage and requires that address's signature for therest of the deployment-wide policy setup. The
AlreadyInitializedguard nowchecks
DataKey::Policyinstead ofDataKey::Admin, sinceAdminis setat construction and always present on a live instance.
Also updated
scripts/testnet-load-v2.shto pass--adminas a constructorargument at deploy time instead of a separate
initializecall.v1 (
contracts/tholos) has the same class of gap in its owninitializeand doesn't have a fix yet — worth mirroring this approach there for
consistency once that issue is opened.
Test plan
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings, andcargo testpass locallyCONTRACT.mdupdated if the public interface changed — N/A, that doc is scoped tocontracts/tholos(v1) onlyscripts/testnet-smoke.shrun against testnet, if this changes contract behavior in a way that affects the deployed flow — N/A, that script only covers v1test_initialize_rejects_caller_other_than_constructor_admin, which mocks auth for an address distinct from the real constructor-time admin and confirmsinitializestill rejects the callcargo test -p tholos-v2: 108 passed, 0 failedcargo build --target wasm32v1-none -p tholos-v2 --release: compiles cleantest_snapshots/test/*.jsonto confirm the mocked-ledger trace now shows two invocations (__constructortheninitialize) instead of one, matching the intended fix