Upgrade governance scope check only blocks two hardcoded addresses, not transitive privilege escalation

[christabel888]

Summary

TargetScope::validate_target_scope (contracts/upgrade/src/scope.rs) blocks an upgrade proposal from targeting exactly two hardcoded addresses — the governance contract and the UpgradeManager itself. docs/contract-privilege-graph.md claims this contract system provides "a formal model of privilege reachability, transitive closure, attack surface analysis" — but the actual check is a two-address equality test with no notion of transitive reachability at all.

Location

contracts/upgrade/src/scope.rs (full file, 34 lines):

pub fn is_restricted_target(env: &Env, target: &Address, governance: &Address) -> bool {
    if target == governance {
        return true;
    }
    if target == &env.current_contract_address() {
        return true;
    }
    false
}

docs/contract-privilege-graph.md, contracts/stellar_insights/src/lib.rs (set_upgrade_manager, governance_upgrade).

Current gap / Motivation

The privilege graph doc's own contract inventory lists StellarInsights::set_upgrade_manager as a privileged entry point requiring "Admin auth for ... upgrade manager setup." That function's whole purpose is to designate which contract address is trusted as the UpgradeManager for future governed upgrades of StellarInsights.

validate_target_scope has no awareness of this. An upgrade proposal targeting StellarInsights — a perfectly legitimate, explicitly-allowed governed target per the doc's own diagram — passes the scope check purely because StellarInsights != governance and StellarInsights != UpgradeManager. But the WASM being installed at that target is attacker/proposer-controlled code (subject to whatever approval process gates execute_upgrade — which is a separate, social/multisig control, not something scope.rs can see). If that approval process is ever compromised, tricked, or simply has a lower bar than the direct-target checks assume, the newly-installed StellarInsights code can call its own (now-controlled) set_upgrade_manager to install a completely different, attacker-controlled UpgradeManager address — one with no scope restrictions at all, since scope.rs's restricted-target list is hardcoded to the current governance/UpgradeManager identities at the time each proposal is validated, not to any invariant about what capabilities a target holds.

In other words: the scope check reasons about identity, not about capability. It cannot express "no governed target may hold or grant the ability to redesignate upgrade authority," which is exactly the kind of transitive-privilege property the accompanying documentation claims to formally model.

The hard part

This is a genuine smart-contract security design problem, not a one-line fix:

1. Defining the actual invariant. What, precisely, must be true of every governed target for the "UpgradeManager and Governance can never be captured via an upgrade" property to hold? Is it "no governed target may call set_upgrade_manager-shaped functions on any contract," "no governed target's WASM may be granted the specific auth scope that gates set_upgrade_manager," or something else? This needs a real answer, not just a bigger blocklist — a blocklist approach (e.g. adding StellarInsights to the restricted list) would break the legitimate use case of upgrading it at all.
2. This can't be solved by static address-equality checks alone, because the whole point of an upgrade is that the target's behavior changes; scope validation happens before the new WASM is even known to be malicious or benign in this respect. A real solution likely needs either (a) a runtime authorization boundary — e.g. the functions that grant/rotate upgrade authority must themselves require an auth scope that a governance_upgrade-installed WASM can never be granted, enforced by something other than scope.rs, or (b) a formally specified allowlist of which specific entry points a governed upgrade may ever indirectly reach, with execute_upgrade (or a pre-flight simulation) verifying the new WASM's exported capability surface against it.
3. Second-order cases. The same class of gap likely exists for any other contract in the privilege graph that holds a capability-granting hook analogous to set_upgrade_manager — this needs to be enumerated exhaustively against the actual contract set (MultisigContract::reconfigure, TimeLockedTransactions, EscrowContract, TokenSwap), not just patched for the one instance found here.
4. Proving the fix, not just asserting it. Given contracts/tests/privilege_escalation_test.rs today only asserts that certain files exist on disk (std::path::Path::new(...).exists()) rather than exercising any actual contract behavior, whatever replaces this needs a real, executable proof: a test that actually deploys the contracts, executes a governed upgrade of a non-governance/non-UpgradeManager target, has that upgraded code attempt to call set_upgrade_manager (or equivalent) to redirect upgrade authority, and asserts this is rejected.

Implementation

• Formally specify the invariant scope.rs is meant to enforce (see above) and document it in docs/contract-privilege-graph.md alongside the existing diagram.
• Extend the authorization model so that capability-granting entry points (set_upgrade_manager and any equivalents found in the audit above) cannot be reached — directly or via freshly-upgraded code — by anything a governed upgrade can install, or add an explicit, tested mechanism that rejects such a reachability path.
• Replace the placebo privilege_escalation_test.rs (both the one in contracts/tests/ and the real one in contracts/upgrade/tests/) with an executable exploit-attempt test per the scenario above.

Acceptance criteria

• The invariant scope.rs enforces is written down precisely, not just "these two addresses are blocked."
• A test deploys a governed target, upgrades it via the real proposal/approval/execute flow, and demonstrates the upgraded code cannot redirect upgrade or governance authority.
• Every contract in the privilege graph is audited for an equivalent capability-granting hook; findings are either fixed or explicitly documented as accepted risk with rationale.
• contracts/tests/privilege_escalation_test.rs and contracts/upgrade/tests/privilege_escalation_test.rs exercise actual contract behavior, not filesystem existence checks.
• Existing cargo test suite stays green.

[oyinade247]

Can I work on this pleaseeeeeeeeee

[Horlarmmy]

Hi again, I'd like to work on this issue. Kindly assign

[GazzyLee]

I’d like to take this on. I’ll first define the exact privilege invariant and audit the contracts for equivalent capability-granting paths, then implement the authorization boundary needed to prevent upgraded code from redirecting governance or upgrade authority.

I’ll also replace the existing placeholder privilege tests with an executable end-to-end exploit scenario covering the real upgrade flow, and document any remaining accepted risks with clear rationale.
please assign