fix(security,#437): stop asset_admin seizing an asset's oracle_authority

[0x-SquidSol]

Fixes #437.

What was wrong

handle_update_asset_authority skips expect_live_authority(&current_value, current.key) — the current holder's consent check — whenever admin_signed. #430 scoped the carve-out to the two insurance legs:

let admin_bypass_permitted = !matches!(
    kind, ASSET_AUTH_INSURANCE | ASSET_AUTH_INSURANCE_OPERATOR
) || current_value == [0u8; 32];

There are five authority kinds. ASSET_AUTH_ORACLE still took the bypass, so asset_admin could take an asset's oracle_authority from a holder that never signed — and that role gates the asset's price surface: ConfigureAuthMark (:13809), ConfigureEwmaMark (:13705), ConfigureHybridOracle (:13583), PushEwmaMark (:13914), PushAuthMark (:13990).

Because ConfigureAuthMark switches oracle_mode to AUTH_MARK (:13812), the attack is not limited to assets already configured that way. The issue measures the full chain on a live market: mark 100 -> 999_000_000.

This also restores the model the repo already documents (README.md:453):

Non-burn transfers require both the current authority and the new key to sign.

The pre-fix oracle leg did not honour that.

The fix

One word: ASSET_AUTH_ORACLE added to the existing exclusion list.

Why not the blanket form

The tempting version is:

let admin_bypass_permitted = current_value == [0u8; 32];

Do not use it. It reintroduces the unrecoverable brick that review of #415 caught. handle_update_asset_authority enforces expect_signer(current), handle_create_lp_vault binds backing_bucket_authority = registry_pda (:14561), and a PDA cannot sign — so the field would become permanently unrotatable once the vault is gone. #424's guard (:12418) is narrower on purpose: it refuses only while the registry is initialized, and that gap is the recovery path.

Measured, under the blanket form:

backing_authority_rotation_is_allowed_when_no_lp_vault_registry_exists
  InstructionError(2, Custom(8))
test result: FAILED. 0 passed; 1 failed

That is the regression test added alongside #424 for exactly this brick. I originally suggested the blanket form in #437 and retracted it there once measured.

A note on the comment

The current_value == [0u8; 32] escape is effectively dead code for the oracle leg — every activation path writes a non-zero oracle_authority, and the zero-burn at :12378 is rejected for every kind except ASSET_AUTH_ADMIN. It is kept for uniformity with the insurance legs, and the code comment says so rather than implying the oracle role can be vacant.

Operational cost — stated plainly

This is the real trade-off and it should be a deliberate choice, not a surprise:

• A lost or compromised oracle key can now be contained but not replaced. Containment is one instruction — ASSET_ACTION_SHUTDOWN (:12912), available to marketauth or asset_admin, moves the asset to RECOVERY, after which require_asset_mark_pushable_view rejects further pushes.
• But RestartAssetOracle preserves oracle_authority (:12658), so bringing the asset back hands the key to the same holder. Full replacement needs ASSET_ACTION_RETIRE then re-ACTIVATE.
• For asset 0 that is impossible: RETIRE rejects asset_index == 0, so asset 0's oracle key is unreplaceable once delegated.

Two things make me think this is still the right trade:

1. It is the same property the already-shipped insurance legs have. fix(security): Live gate on maintenance-fee updates (#428) + block asset_admin insurance seizure (#416, #417) #430 gave ASSET_AUTH_INSURANCE and ASSET_AUTH_INSURANCE_OPERATOR exactly this treatment, and RETIRE's asset_index == 0 rejection applies identically. This extends an accepted trade-off rather than introducing a new class of risk.
2. The pre-fix power was the larger hazard. On a permissionless asset the activator holds asset_admin, so anyone delegating pricing to a third-party feed provider had no protection at all — the slot owner could take the oracle and print a mark on a live market.

No routine operation breaks: rotation of a hot pusher key is initiated by the outgoing key with the incoming key co-signing, asset_admin is not a party, and activation-time provisioning names oracle_authority inline and is untouched.

Suggested follow-up (not in this PR): give RestartAssetOracle an oracle_authority parameter. It is already asset_admin-gated (:12641) and only reachable from RECOVERY — i.e. after the asset is already frozen — so installing a fresh key there would restore eviction without reopening the live-market seizure this PR closes. Happy to open it separately.

Scope

Not in this PR: ASSET_AUTH_BACKING_BUCKET is still exposed when the holder is an ordinary signable key rather than the registry PDA — #424's guard only covers the PDA case, and that field gates WithdrawBackingBucket (:10221), which moves tokens. It is the same class and wants a carve-out shaped like current_value == registry_pda so the PDA recovery path survives. I am filing it separately rather than widening a security fix past the issue it references.

Also worth recording, since it qualifies the issue's framing: oracle_authority is not the only price lever asset_admin holds. ASSET_ACTION_SHUTDOWN followed by RestartAssetOracle(initial_price) rebuilds the profile at an arbitrary price. That path is far weaker — the engine's require_empty_asset_lifecycle_state requires k_long == 0 && k_short == 0, so the asset must have no open interest, meaning there are no positions to misprice. The oracle leg closed here is the one that works on a live asset with open positions.

Testing

Two regression tests, both written first and watched to fail on unfixed main, while the two existing insurance tests pass throughout — which is what makes the differential meaningful:

test v16_wrapper_asset_admin_cannot_seize_insurance_operator_from_holder ... ok
test v16_wrapper_asset_admin_cannot_seize_insurance_authority_from_holder ... ok
test v16_wrapper_asset_admin_cannot_reach_the_mark_without_the_oracle_authority ... FAILED
test v16_wrapper_asset_admin_cannot_seize_oracle_authority_from_holder ... FAILED

With the fix, all four pass. The first test is a differential over kind with identical accounts, so a future carve-out that re-opens one leg shows up immediately.

suite	result
v16_wrapper	223 passed / 19 failed (baseline main: 221 / 19)
v16_fork_lp_vault_deposit	10 / 0
v16_fork_lp_vault_create	5 / 0
v16_fork_lp_vault_redeem	27 / 0
v16_fork_lp_vault_admin	8 / 0
v16_fork_lp_vault_state_tests	21 / 0
v16_authority_binding_canary	5 / 0
v16_fork_adversarial	14 / 0

The v16_wrapper failing set matches tests/KNOWN_FAILING.txt exactly — 19 documented, 19 actual, none undocumented and none newly passing — which is that file's stated criterion for green. The delta over baseline is exactly the two tests added here.

BPF built with cargo build-sbf --no-default-features --tools-version v1.52 before running the fork suites; without it those suites fail spuriously on a stale artifact.

All src/v16_program.rs line references above are against main (a1a8168), not this branch — the change adds 11 lines at :12388, so anything below that shifts here.

Summary by CodeRabbit

• Bug Fixes

  • Strengthened authorization checks for asset and oracle authority operations.
  • Prevented unauthorized replacement of authority holders and modification of oracle marks.
  • Ensured rejected operations leave market state, authority ownership, and oracle marks unchanged.

• Tests

  • Added regression coverage for unauthorized asset-authority seizures and oracle mark changes.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d623914a-2928-4d24-8616-d0277b08af04

📥 Commits

Reviewing files that changed from the base of the PR and between a1a8168 and fd57c33.

📒 Files selected for processing (2)

• src/v16_program.rs
• tests/v16_wrapper.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The asset authority update path now requires holder consent for occupied oracle authority. New regression tests cover unauthorized authority takeover, oracle mark configuration, and oracle mark updates.

Changes

Authority consent enforcement

Layer / File(s)	Summary
Oracle authority consent guard src/v16_program.rs	The admin bypass exclusion now includes ASSET_AUTH_ORACLE. Occupied oracle authority uses live authority validation, while vacant authority can still be assigned.
Authority takeover and oracle mark tests tests/v16_wrapper.rs	Tests reject unsigned takeover of insurance, insurance-operator, and oracle authority. Additional tests reject unauthorized oracle mark configuration and updates, and verify that authority and marks remain unchanged.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to fd57c

The change narrows administrative authority replacement so oracle ownership still requires the current holder’s consent while preserving the existing recovery exception. No actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: dcccrypto, ayomisco

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR prevents unconsented seizure of ASSET_AUTH_ORACLE and blocks related oracle mark manipulation. It does not implement the linked issue's required ASSET_AUTH_BACKING_BUCKET protection or its regr…	Add occupied ASSET_AUTH_BACKING_BUCKET protection and regression coverage. Ensure the authority-bypass logic also protects future occupied authority kinds, or split those requirements into a separately linked issue.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the security fix that prevents asset administrators from seizing an asset's oracle authority.
Out of Scope Changes check	✅ Passed	The source change and regression tests directly support the oracle-authority security objective. No unrelated code changes are identified.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…

Full details: Linked Issues check

Explanation

The PR prevents unconsented seizure of ASSET_AUTH_ORACLE and blocks related oracle mark manipulation. It does not implement the linked issue's required ASSET_AUTH_BACKING_BUCKET protection or its regression test, and it does not cover future authority kinds.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 too large.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.97.1)

Clippy execution failed

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[dcccrypto]

Thanks @0x-SquidSol — the finding was right and it shipped, just via a broader guard than this patch.

#437/#439 are fixed and deployed (wrapper 02326f4f, now on main). Closing this as superseded, not rejected.

Why the merged fix is different

This PR adds ASSET_AUTH_ORACLE to the deny-list:

let admin_bypass_permitted = !matches!(
    kind,
    ASSET_AUTH_INSURANCE | ASSET_AUTH_INSURANCE_OPERATOR | ASSET_AUTH_ORACLE
) || current_value == [0u8; 32];

That closes the oracle leg, but leaves the bypass open for ASSET_AUTH_BACKING_BUCKET — which #439 reports through — and for ASSET_AUTH_ADMIN. A deny-list has to be extended once per leg, and the same class had already been reported twice through two different legs.

The merged fix inverts it into an allowlist, so the bypass is denied for every kind unless there is genuinely no holder to defend:

let admin_bypass_permitted = admin_signed
    && (current_value == [0u8; 32] || { /* LP-vault registry PDA, BACKING_BUCKET only */ });

The registry-PDA carve-out exists because LP-vault custody (#424) needs it and the holder there is a PDA that cannot sign.

Your analysis was also ahead of us in one place

The note that current_value == [0u8; 32] is unreachable for ORACLE in practice — every activation path writes a non-zero oracle_authority — is correct and we had not written it down.

One thing worth passing on

Upstream aeyakovenko/percolator-prog fixed this same family in 255e56ee and went stricter still: it deletes the bypass outright, so holder consent is unconditional. That branch also carries an attack neither of us had considered — a pre-signed admin assignment landing after a newer holder-signed handoff, reviving a displaced key. We have since verified our guard blocks it and added a test, but only because we went and looked.

Equivalent coverage to your differential test now lives in our authorization gate, which sweeps every authority kind rather than the two the old guard named.