fix(soroban): separate sentinel emergency authority#427
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds sentinel-only direct setters and new timelocked governance action kinds, refactors vault policy/authorization to separate sentinel immediate actions from governance-timelocked flows, tightens execute_governance routing, and updates tests, docs, and deployment recipes accordingly. ChangesGovernance control-plane & docs
Governance contract
Vault contract refactor
Tests & MockVault
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Gov as GovernanceContract
participant Vault as VaultContract
participant Storage as VaultStorage
rect rgba(100, 200, 150, 0.5)
Note over Caller,Storage: Sentinel immediate action
Caller->>Gov: set_paused(caller, true)
Gov->>Gov: require_sentinel()
Gov->>Vault: execute_vault_governance_action_as_caller(paused=true)
Vault->>Storage: persist paused = true
end
rect rgba(100, 150, 200, 0.5)
Note over Caller,Storage: Timelocked governance flow
Caller->>Gov: submit_set_paused(caller, false)
Gov->>Gov: decide_submission() -> TimelockDecision::Timelocked
Gov->>Storage: enqueue pending proposal
note right of Storage: after maturity & acceptance
Gov->>Vault: invoke unpause (governance path)
Vault->>Storage: persist paused = false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
|
@coderabbitai review |
|
@coderabbitai for the benefit of the human reviewers - please output a description of this PR and any critique on the usability of the design when you are reviewing. |
✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the changed files from this PR to give a thorough description and design critique. 🏁 Script executed: Length of output: 222 🏁 Script executed: Length of output: 30520
PR DescriptionThis PR implements the A-003 "Model B" authority split: previously the governance contract was implicitly treated as capable of performing emergency actions (pause, restriction tightening) by routing them through the same timelocked proposal queue. This PR makes the two authority domains explicit and disjoint:
Governance contract changes (
Runtime changes (
Tests: Two renamed governance tests, plus two new runtime integration tests ( Design Critique1. Runtime does not enforce "caller must be Sentinel" for emergency operations — only "caller must be signed"This is the most significant usability/security design point for reviewers to evaluate. In require_signed(&caller);This checks that Consequence: Any externally-owned account or contract that can craft a transaction can call The previous design called A defense-in-depth fix would be to add a 2. Governance's
|
1 similar comment
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the changed files from this PR to give a thorough description and design critique. 🏁 Script executed: Length of output: 222 🏁 Script executed: Length of output: 30520
PR DescriptionThis PR implements the A-003 "Model B" authority split: previously the governance contract was implicitly treated as capable of performing emergency actions (pause, restriction tightening) by routing them through the same timelocked proposal queue. This PR makes the two authority domains explicit and disjoint:
Governance contract changes (
Runtime changes (
Tests: Two renamed governance tests, plus two new runtime integration tests ( Design Critique1. Runtime does not enforce "caller must be Sentinel" for emergency operations — only "caller must be signed"This is the most significant usability/security design point for reviewers to evaluate. In require_signed(&caller);This checks that Consequence: Any externally-owned account or contract that can craft a transaction can call The previous design called A defense-in-depth fix would be to add a 2. Governance's
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract/vault/soroban/governance/src/lib.rs`:
- Around line 217-251: The set_paused and set_restrictions entrypoints bypass
the abdication guard, so add the same abdication check used by submit to make
those actions irreversible: call the existing require_not_abdicated (or
equivalent) check early in both functions (before executing any actions or
calling execute_vault_governance_action_as_caller) using the appropriate
GovernanceActionKind for each (GovernanceActionKind::Pause in set_paused and
GovernanceActionKind::Restrictions in set_restrictions) so that after
abdicate(...) the sentinel cannot still immediately perform those actions.
- Around line 233-251: The set_restrictions entrypoint currently forwards any
proposed mode/accounts to the vault; modify set_restrictions so it first loads
the current values from storage (DataKey::CurrentRestrictionMode and
DataKey::CurrentRestrictionAccounts) and compares them to the incoming mode and
accounts using RestrictionMode and account-set logic, and return an error
(GovernanceError) if the transition is not a tightening (e.g., disallow
switching to RestrictionMode::None, disallow enlarging a blacklist or enlarging
a whitelist set) before calling execute_vault_governance_action_as_caller;
ensure you use the existing RestrictionMode::from_u32 conversion and
Equality/ordering semantics you define to determine "tightening" and reference
the same DataKey symbols and function names (set_restrictions,
execute_vault_governance_action_as_caller) when implementing the guard.
In `@contract/vault/soroban/README.md`:
- Around line 77-85: The README omits that execute_governance is unauthenticated
at its entrypoint; update the vault/soroban README section to explicitly state
that execute_governance() decodes and dispatches GovernanceCommand variants
without performing an outer auth check, and that each privileged handler inside
the runtime (the per-variant GovernanceCommand handlers) must perform its own
authentication/authorization and timelock checks; mention GovernanceCommand and
execute_governance by name so reviewers know to enforce auth inside handlers
rather than relying on the router.
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 611-613: The current branch for GOVERNANCE_POLICY_KIND_PAUSED
treats any non-zero mode as "true", turning malformed values into a valid pause
request; change the code to explicitly validate mode is present and equal to 0
or 1. Replace the inline expression using mode.ok_or(...)? != 0 with: extract
mode via let mode_val = mode.ok_or(ContractError::InvalidInput)?; if mode_val !=
0 && mode_val != 1 { return Err(ContractError::InvalidInput); } then call
apply_paused_policy(env, caller, mode_val != 0). This keeps
GOVERNANCE_POLICY_KIND_PAUSED, apply_paused_policy, mode, caller and env as the
reference points.
In `@contract/vault/soroban/src/tests.rs`:
- Around line 2955-3095: The test currently only asserts None->Blacklist and
Blacklist->None transitions; expand it into a parameterized matrix covering all
GovernanceRestrictions transitions (None, Blacklist, Whitelist) and
subset/superset changes so determine_relaxed(...) behavior is fully exercised.
Add rstest-style cases or a loop that calls execute_governance_command (and
direct contract invoke via Symbol::new("execute_governance")) for each prior/new
Restrictions variant (including empty vs non-empty account lists and
whitelist↔blacklist flips), then assert Storage::load_restrictions(&storage) and
the expected authorized caller (governance vs sentinel) match for each
transition; reference GovernanceRestrictions::determine_relaxed,
GovernanceCommand::SetGovernancePolicy, GOVERNANCE_POLICY_KIND_RESTRICTIONS,
execute_governance_command, and Restrictions/Storage::load_restrictions to
locate and implement the cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a82b41ef-2909-4215-bafb-5da75714cebf
📒 Files selected for processing (5)
contract/vault/soroban/README.mdcontract/vault/soroban/governance/src/lib.rscontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/tests.rs
| - Vault-bound governance actions cross the boundary through a single bridge: | ||
| `execute_governance(env, caller, payload)`. The payload is a `GovernanceCommand` that the | ||
| runtime decodes and dispatches to the corresponding internal config/policy/state helpers. | ||
| - Emergency pause and restriction tightening are immediate Sentinel actions. Unpause and | ||
| relaxing/removing restrictions are governance actions and must pass through the configured | ||
| timelock before the runtime applies them. | ||
| - `execute(payload)` remains for user flows and for the retained execute-path config subset | ||
| (`ALLOCATORS`, `ALLOWED_ADAPTERS`, `VIRTUAL_OFFSETS`). Vault-bound governance mutations use | ||
| `execute_governance`, not the generic user-flow command path. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Spell out that execute_governance is authless at the entrypoint.
This section explains the bridge, but not the easy-to-miss part: the runtime only authenticates inside each GovernanceCommand handler. Making that explicit here would save future reviewers from assuming outer routing protects new variants.
Based on learnings: execute_governance() decodes and dispatches without an auth check at the entrypoint; every privileged GovernanceCommand path must enforce its own auth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/vault/soroban/README.md` around lines 77 - 85, The README omits that
execute_governance is unauthenticated at its entrypoint; update the
vault/soroban README section to explicitly state that execute_governance()
decodes and dispatches GovernanceCommand variants without performing an outer
auth check, and that each privileged handler inside the runtime (the per-variant
GovernanceCommand handlers) must perform its own authentication/authorization
and timelock checks; mention GovernanceCommand and execute_governance by name so
reviewers know to enforce auth inside handlers rather than relying on the
router.
| #[test] | ||
| fn test_execute_governance_separates_sentinel_tightening_from_governance_relaxation() { | ||
| use soroban_sdk::{IntoVal, Symbol}; | ||
|
|
||
| let env = Env::default(); | ||
| env.mock_all_auths_allowing_non_root_auth(); | ||
| let contract_id = env.register(SorobanVaultContract, ()); | ||
| let curator = SdkAddress::generate(&env); | ||
| let governance = SdkAddress::generate(&env); | ||
| let sentinel = SdkAddress::generate(&env); | ||
| let restricted = SdkAddress::generate(&env); | ||
| let asset = SdkAddress::generate(&env); | ||
| let share = SdkAddress::generate(&env); | ||
|
|
||
| env.as_contract(&contract_id, || { | ||
| SorobanVaultContract::initialize( | ||
| env.clone(), | ||
| curator, | ||
| governance.clone(), | ||
| asset, | ||
| share, | ||
| 0, | ||
| 0, | ||
| ) | ||
| .unwrap(); | ||
| }); | ||
|
|
||
| execute_governance_command( | ||
| &env, | ||
| &contract_id, | ||
| &governance, | ||
| &GovernanceCommand::SetGovernanceConfig { | ||
| kind: GOVERNANCE_CONFIG_KIND_SENTINEL, | ||
| primary: Some(sdk_text(&sentinel)), | ||
| many: None, | ||
| value_a: None, | ||
| value_b: None, | ||
| }, | ||
| ); | ||
|
|
||
| let governance_tightening = env.try_invoke_contract::<(), crate::error::ContractError>( | ||
| &contract_id, | ||
| &Symbol::new(&env, "execute_governance"), | ||
| ( | ||
| &governance, | ||
| &Bytes::from_slice( | ||
| &env, | ||
| &GovernanceCommand::SetGovernancePolicy { | ||
| kind: GOVERNANCE_POLICY_KIND_RESTRICTIONS, | ||
| target_ids: None, | ||
| mode: Some(1), | ||
| accounts: Some(alloc::vec![sdk_text(&restricted)]), | ||
| market_id: None, | ||
| cap_group_id: None, | ||
| value: None, | ||
| value_b: None, | ||
| value_c: None, | ||
| } | ||
| .encode(), | ||
| ), | ||
| ) | ||
| .into_val(&env), | ||
| ); | ||
| assert_eq!( | ||
| governance_tightening, | ||
| Err(Ok(crate::error::ContractError::Unauthorized)) | ||
| ); | ||
|
|
||
| execute_governance_command( | ||
| &env, | ||
| &contract_id, | ||
| &sentinel, | ||
| &GovernanceCommand::SetGovernancePolicy { | ||
| kind: GOVERNANCE_POLICY_KIND_RESTRICTIONS, | ||
| target_ids: None, | ||
| mode: Some(1), | ||
| accounts: Some(alloc::vec![sdk_text(&restricted)]), | ||
| market_id: None, | ||
| cap_group_id: None, | ||
| value: None, | ||
| value_b: None, | ||
| value_c: None, | ||
| }, | ||
| ); | ||
| env.as_contract(&contract_id, || { | ||
| let storage = SorobanStorage::new(&env); | ||
| assert!(matches!( | ||
| Storage::load_restrictions(&storage).unwrap(), | ||
| Some(Restrictions::Blacklist(_)) | ||
| )); | ||
| }); | ||
|
|
||
| let sentinel_relaxation = env.try_invoke_contract::<(), crate::error::ContractError>( | ||
| &contract_id, | ||
| &Symbol::new(&env, "execute_governance"), | ||
| ( | ||
| &sentinel, | ||
| &Bytes::from_slice( | ||
| &env, | ||
| &GovernanceCommand::SetGovernancePolicy { | ||
| kind: GOVERNANCE_POLICY_KIND_RESTRICTIONS, | ||
| target_ids: None, | ||
| mode: Some(0), | ||
| accounts: Some(alloc::vec![]), | ||
| market_id: None, | ||
| cap_group_id: None, | ||
| value: None, | ||
| value_b: None, | ||
| value_c: None, | ||
| } | ||
| .encode(), | ||
| ), | ||
| ) | ||
| .into_val(&env), | ||
| ); | ||
| assert_eq!( | ||
| sentinel_relaxation, | ||
| Err(Ok(crate::error::ContractError::Unauthorized)) | ||
| ); | ||
|
|
||
| execute_governance_command( | ||
| &env, | ||
| &contract_id, | ||
| &governance, | ||
| &GovernanceCommand::SetGovernancePolicy { | ||
| kind: GOVERNANCE_POLICY_KIND_RESTRICTIONS, | ||
| target_ids: None, | ||
| mode: Some(0), | ||
| accounts: Some(alloc::vec![]), | ||
| market_id: None, | ||
| cap_group_id: None, | ||
| value: None, | ||
| value_b: None, | ||
| value_c: None, | ||
| }, | ||
| ); | ||
| env.as_contract(&contract_id, || { | ||
| let storage = SorobanStorage::new(&env); | ||
| assert_eq!(Storage::load_restrictions(&storage).unwrap(), None); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a relaxation/tightening matrix instead of only the None/Blacklist path.
The new branch hinges on GovernanceRestrictions::determine_relaxed(...), but these tests only cover None -> Blacklist and Blacklist -> None. They will not catch whitelist/blacklist flips or subset/superset changes, which are exactly where the authorized caller changes.
As per coding guidelines: "Write comprehensive unit tests for new, non-trivial Rust logic; prefer #[rstest] parameterization when it improves coverage and keeps cases readable."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/vault/soroban/src/tests.rs` around lines 2955 - 3095, The test
currently only asserts None->Blacklist and Blacklist->None transitions; expand
it into a parameterized matrix covering all GovernanceRestrictions transitions
(None, Blacklist, Whitelist) and subset/superset changes so
determine_relaxed(...) behavior is fully exercised. Add rstest-style cases or a
loop that calls execute_governance_command (and direct contract invoke via
Symbol::new("execute_governance")) for each prior/new Restrictions variant
(including empty vs non-empty account lists and whitelist↔blacklist flips), then
assert Storage::load_restrictions(&storage) and the expected authorized caller
(governance vs sentinel) match for each transition; reference
GovernanceRestrictions::determine_relaxed,
GovernanceCommand::SetGovernancePolicy, GOVERNANCE_POLICY_KIND_RESTRICTIONS,
execute_governance_command, and Restrictions/Storage::load_restrictions to
locate and implement the cases.
carrion256
force-pushed
the
audit/governance-control-plane
branch
2 times, most recently
from
dc05a58 to
488efb4
Compare
carrion256
force-pushed
the
audit/governance-control-plane
branch
from
f8bde99 to
36503d8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contract/vault/soroban/governance/src/tests.rs (1)
1674-1696:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject governance-side restrictions tightening at submission time.
This test currently treats a governance-authored tightening as a valid pending proposal that must be manually revoked. But the runtime tests in
contract/vault/soroban/src/tests.rsalready prove governance callers are rejected for tightening withUnauthorized, so this is a dead-end proposal path. Please fail fast here or route tightenings exclusively through the sentinel-only entrypoint instead of queueing something that can never execute.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/soroban/governance/src/tests.rs` around lines 1674 - 1696, The test incorrectly assumes a governance-submitted tightening will create a pending proposal; instead change the submit_set_restrictions call (SorobanVaultGovernanceContract::submit_set_restrictions invoked in the env.as_contract closure that sets governance_tightening_id) to expect failure: call it and assert it returns an Err/Unauthorized (unwrap_err or pattern-match the specific error) and remove the subsequent pending_ids and revoke logic for that tightening; alternatively, if you intend to exercise a valid tightening path, route the call through the sentinel-only entrypoint rather than SorobanVaultGovernanceContract::submit_set_restrictions so no dead pending_id is produced.contract/vault/soroban/src/contract/entrypoints.rs (1)
573-603:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKeep config changes and skim governance-only.
execute_governance()now admits any control-plane caller, butset_governance_config_impl()andskim_impl()no longer re-check governance identity. The new sentinel-path tests show a sentinel can reach this entrypoint, so a configured sentinel can now callexecute_governancedirectly to rotate curator/governance/allocators/adapters or skim arbitrary tokens immediately, bypassing the timelocked governance path.Suggested fix
fn execute_governance_command( env: &Env, caller: soroban_sdk::Address, command: GovernanceCommand, caller_preauthorized: bool, ) -> Result<(), ContractError> { + let ensure_governance = || -> Result<(), ContractError> { + if caller_preauthorized { + ensure_governance_identity(env, &caller) + } else { + require_governance(env, &caller) + } + }; + match command { GovernanceCommand::SetGovernanceConfig { kind, primary, many, value_a, value_b, } => { + ensure_governance()?; let primary = primary .as_ref() .map(|value| address_from_alloc_string(env, value)) .transpose()?; let many = many @@ set_governance_config_impl(env, caller, kind, primary, many, value_a, value_b) } GovernanceCommand::SetGovernancePolicy { kind, target_ids, @@ caller_preauthorized, ) } GovernanceCommand::Skim { token } => { + ensure_governance()?; skim_impl(env, caller, address_from_alloc_string(env, &token)?) } } }Also applies to: 692-713, 945-953
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/soroban/src/contract/entrypoints.rs` around lines 573 - 603, The set_governance_config_impl and skim_impl paths are missing an internal governance identity check, letting sentinels or other control-plane callers bypass timelocked governance via execute_governance; fix by enforcing that only the on-chain governance address can perform these sensitive changes: add a require_governance-style check (compare caller to stored governance address) at the top of set_governance_config_impl and skim_impl (and the similar entrypoint blocks referenced around the other ranges), or alternatively move the governance check into execute_governance so it rejects non-governance callers before delegating to apply_* functions (apply_curator_config, apply_governance_config, apply_allocators_config, apply_allowed_adapters_config, apply_sentinel_config, apply_guardians_config, apply_skim_recipient_config, apply_virtual_offsets_config); ensure the check uses the same canonical governance lookup used elsewhere to prevent privilege escalation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/vault/soroban/governance/src/lib.rs`:
- Around line 228-243: The set_paused function currently accepts a bool but
immediately rejects false, making half the input space impossible; change the
ABI to use explicit entrypoints/types instead: replace set_paused(env, caller,
paused: bool) and the GovernanceAction::SetPaused variant with two distinct
functions (e.g., pause(env, caller) and submit_unpause(env, caller)) or with
separate action enums (e.g., GovernanceAction::Pause and
GovernanceAction::SubmitUnpause) so callers cannot pass an invalid boolean;
update calls to execute_vault_governance_action_as_caller and any storage keys
like DataKey::CurrentPaused to use the new functions/variants and similarly
refactor the timelocked path (the logic currently around the other branch) to
accept only the valid transition.
- Around line 228-243: In set_paused, avoid dispatching the vault call for a
no-op: read the current paused flag from storage (DataKey::CurrentPaused) at the
start of the function and if the requested paused value equals the current value
return Err(GovernanceError::NoChange) immediately; do this before calling
execute_vault_governance_action_as_caller and before writing storage so
redundant pauses/unpauses are short-circuited.
- Around line 245-267: The code stores and compares raw Vec<Address> for
restriction members which allows reorders/duplicates to be treated as changes;
in set_restrictions canonicalize the accounts before any equality checks, action
construction, and persistence: collapse duplicates and sort (or otherwise
produce a deterministic set) and if mode == RestrictionMode::None replace the
canonical accounts with an empty Vec; use that canonicalized accounts value for
GovernanceAction::SetRestrictions, for the call to
restrictions_change_is_relaxed, and when writing
DataKey::CurrentRestrictionAccounts (and mirror this canonicalization in the
other equivalent places referenced so storage/comparison logic is consistent).
In `@contract/vault/soroban/justfile`:
- Around line 388-397: The current justfile deploy snippet accepts non-numeric
SOROBAN_GOV_TIMELOCK_NS and fails later; add a numeric validation step after
reading timelock_ns to ensure it is a non-negative integer (e.g. use a regex
like '^[0-9]+$' or a case statement) and if the value is non-numeric print an
explanatory error mentioning SOROBAN_GOV_TIMELOCK_NS and
SOROBAN_ALLOW_ZERO_GOV_TIMELOCK and exit 1; update the block that sets
timelock_ns and checks for empty/zero to first test numericness (then continue
with the existing zero-check that relies on SOROBAN_ALLOW_ZERO_GOV_TIMELOCK).
In `@contract/vault/soroban/STRIDE.md`:
- Line 120: Update the I8 table row for execute_governance(payload) to reflect
the sentinel split: change the auth column from just “require_auth(caller) +
governance check” to explicitly state both governance and sentinel-authorized
paths (e.g., “require_auth(caller) + governance OR sentinel-authorized checks
(pause/tighten paths)”), and ensure the description mentions which actions
(pause, tightening, etc.) can be invoked via sentinel authorization so the
privileged surface accurately reflects execute_governance(payload) and related
governance/sentinel behavior.
---
Outside diff comments:
In `@contract/vault/soroban/governance/src/tests.rs`:
- Around line 1674-1696: The test incorrectly assumes a governance-submitted
tightening will create a pending proposal; instead change the
submit_set_restrictions call
(SorobanVaultGovernanceContract::submit_set_restrictions invoked in the
env.as_contract closure that sets governance_tightening_id) to expect failure:
call it and assert it returns an Err/Unauthorized (unwrap_err or pattern-match
the specific error) and remove the subsequent pending_ids and revoke logic for
that tightening; alternatively, if you intend to exercise a valid tightening
path, route the call through the sentinel-only entrypoint rather than
SorobanVaultGovernanceContract::submit_set_restrictions so no dead pending_id is
produced.
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 573-603: The set_governance_config_impl and skim_impl paths are
missing an internal governance identity check, letting sentinels or other
control-plane callers bypass timelocked governance via execute_governance; fix
by enforcing that only the on-chain governance address can perform these
sensitive changes: add a require_governance-style check (compare caller to
stored governance address) at the top of set_governance_config_impl and
skim_impl (and the similar entrypoint blocks referenced around the other
ranges), or alternatively move the governance check into execute_governance so
it rejects non-governance callers before delegating to apply_* functions
(apply_curator_config, apply_governance_config, apply_allocators_config,
apply_allowed_adapters_config, apply_sentinel_config, apply_guardians_config,
apply_skim_recipient_config, apply_virtual_offsets_config); ensure the check
uses the same canonical governance lookup used elsewhere to prevent privilege
escalation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb48b16a-06ea-45dc-b1e0-59b64f73e6be
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
contract/vault/soroban/Cargo.tomlcontract/vault/soroban/README.mdcontract/vault/soroban/STRIDE.mdcontract/vault/soroban/governance/src/lib.rscontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/governance/src/types.rscontract/vault/soroban/justfilecontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/helpers.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/integration_tests.rs
| pub fn set_paused(env: Env, caller: Address, paused: bool) -> Result<(), GovernanceError> { | ||
| extend_instance_ttl(&env); | ||
| require_sentinel(&env, &caller)?; | ||
| if !paused { | ||
| return Err(GovernanceError::InvalidInput); | ||
| } | ||
|
|
||
| let action = GovernanceAction::SetPaused(paused); | ||
| require_not_abdicated(&env, &action)?; | ||
| let vault = get_address(&env, DataKey::Vault)?; | ||
| execute_vault_governance_action_as_caller(&env, &vault, &caller, &action)?; | ||
| env.storage() | ||
| .instance() | ||
| .set(&DataKey::CurrentPaused, &paused); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Remove the impossible half of the pause ABI.
set_paused rejects false, while the timelocked path now rejects true. That leaves both public paths exposing a bool where half of the domain is intentionally invalid. Split this into distinct pause() / submit_unpause() entrypoints, or separate action variants, so callers cannot discover the rule only via an on-chain failure.
As per coding guidelines: "Prefer parsing over validation in Rust code; express invariants in types wherever practical, and make invalid states unrepresentable."
Also applies to: 759-774
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/vault/soroban/governance/src/lib.rs` around lines 228 - 243, The
set_paused function currently accepts a bool but immediately rejects false,
making half the input space impossible; change the ABI to use explicit
entrypoints/types instead: replace set_paused(env, caller, paused: bool) and the
GovernanceAction::SetPaused variant with two distinct functions (e.g.,
pause(env, caller) and submit_unpause(env, caller)) or with separate action
enums (e.g., GovernanceAction::Pause and GovernanceAction::SubmitUnpause) so
callers cannot pass an invalid boolean; update calls to
execute_vault_governance_action_as_caller and any storage keys like
DataKey::CurrentPaused to use the new functions/variants and similarly refactor
the timelocked path (the logic currently around the other branch) to accept only
the valid transition.
Short-circuit redundant pauses before calling the vault.
This path never checks CurrentPaused, so calling set_paused(true) while already paused still dispatches execute_governance instead of returning NoChange like the timelocked path does at Line 769. Load the current flag and reject the no-op locally before making the privileged external call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/vault/soroban/governance/src/lib.rs` around lines 228 - 243, In
set_paused, avoid dispatching the vault call for a no-op: read the current
paused flag from storage (DataKey::CurrentPaused) at the start of the function
and if the requested paused value equals the current value return
Err(GovernanceError::NoChange) immediately; do this before calling
execute_vault_governance_action_as_caller and before writing storage so
redundant pauses/unpauses are short-circuited.
| pub fn set_restrictions( | ||
| env: Env, | ||
| caller: Address, | ||
| mode: u32, | ||
| accounts: Vec<Address>, | ||
| ) -> Result<(), GovernanceError> { | ||
| extend_instance_ttl(&env); | ||
| require_sentinel(&env, &caller)?; | ||
| let mode = RestrictionMode::from_u32(mode)?; | ||
| let action = GovernanceAction::SetRestrictions(mode, accounts.clone()); | ||
| require_not_abdicated(&env, &action)?; | ||
| if restrictions_change_is_relaxed(&env, mode, &accounts) { | ||
| return Err(GovernanceError::InvalidInput); | ||
| } | ||
| let vault = get_address(&env, DataKey::Vault)?; | ||
| execute_vault_governance_action_as_caller(&env, &vault, &caller, &action)?; | ||
| env.storage() | ||
| .instance() | ||
| .set(&DataKey::CurrentRestrictionMode, &mode); | ||
| env.storage() | ||
| .instance() | ||
| .set(&DataKey::CurrentRestrictionAccounts, &accounts); | ||
| Ok(()) |
There was a problem hiding this comment.
Canonicalize restriction members before equality and persistence.
The effective restriction policy is set-like, but these paths still compare and store a raw Vec<Address>. Reordering the same addresses, submitting duplicates, or changing accounts while mode == RestrictionMode::None can be treated as a fresh governance change even though restriction_snapshot collapses those cases to the same effective policy. Normalize to a canonical set form first, and clear accounts entirely for None.
As per coding guidelines: "Prefer stronger types over loosely constrained values in Rust: enums over well-known strings, sets over vectors when uniqueness matters, dedicated newtypes over primitive obsession, and structured state machines over ad hoc flags."
Also applies to: 837-854, 1617-1648
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/vault/soroban/governance/src/lib.rs` around lines 245 - 267, The
code stores and compares raw Vec<Address> for restriction members which allows
reorders/duplicates to be treated as changes; in set_restrictions canonicalize
the accounts before any equality checks, action construction, and persistence:
collapse duplicates and sort (or otherwise produce a deterministic set) and if
mode == RestrictionMode::None replace the canonical accounts with an empty Vec;
use that canonicalized accounts value for GovernanceAction::SetRestrictions, for
the call to restrictions_change_is_relaxed, and when writing
DataKey::CurrentRestrictionAccounts (and mirror this canonicalization in the
other equivalent places referenced so storage/comparison logic is consistent).
| timelock_ns="${SOROBAN_GOV_TIMELOCK_NS:-}"; \ | ||
| if [ -z "$timelock_ns" ]; then \ | ||
| echo "Error: set SOROBAN_GOV_TIMELOCK_NS before deploying governance."; \ | ||
| echo "For ephemeral local testing only, set SOROBAN_GOV_TIMELOCK_NS=0 and SOROBAN_ALLOW_ZERO_GOV_TIMELOCK=1."; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| if [ "$timelock_ns" = "0" ] && [ "${SOROBAN_ALLOW_ZERO_GOV_TIMELOCK:-0}" != "1" ]; then \ | ||
| echo "Error: zero governance timelock requires SOROBAN_ALLOW_ZERO_GOV_TIMELOCK=1."; \ | ||
| exit 1; \ | ||
| fi; \ |
There was a problem hiding this comment.
Validate SOROBAN_GOV_TIMELOCK_NS is numeric before deploy.
Right now a non-numeric value passes these checks and fails later with a less actionable CLI error.
Suggested patch
timelock_ns="${SOROBAN_GOV_TIMELOCK_NS:-}"; \
if [ -z "$timelock_ns" ]; then \
echo "Error: set SOROBAN_GOV_TIMELOCK_NS before deploying governance."; \
echo "For ephemeral local testing only, set SOROBAN_GOV_TIMELOCK_NS=0 and SOROBAN_ALLOW_ZERO_GOV_TIMELOCK=1."; \
exit 1; \
fi; \
+ if ! printf '%s' "$timelock_ns" | grep -Eq '^[0-9]+$'; then \
+ echo "Error: SOROBAN_GOV_TIMELOCK_NS must be an unsigned integer (nanoseconds)."; \
+ exit 1; \
+ fi; \
if [ "$timelock_ns" = "0" ] && [ "${SOROBAN_ALLOW_ZERO_GOV_TIMELOCK:-0}" != "1" ]; then \
echo "Error: zero governance timelock requires SOROBAN_ALLOW_ZERO_GOV_TIMELOCK=1."; \
exit 1; \
fi; \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| timelock_ns="${SOROBAN_GOV_TIMELOCK_NS:-}"; \ | |
| if [ -z "$timelock_ns" ]; then \ | |
| echo "Error: set SOROBAN_GOV_TIMELOCK_NS before deploying governance."; \ | |
| echo "For ephemeral local testing only, set SOROBAN_GOV_TIMELOCK_NS=0 and SOROBAN_ALLOW_ZERO_GOV_TIMELOCK=1."; \ | |
| exit 1; \ | |
| fi; \ | |
| if [ "$timelock_ns" = "0" ] && [ "${SOROBAN_ALLOW_ZERO_GOV_TIMELOCK:-0}" != "1" ]; then \ | |
| echo "Error: zero governance timelock requires SOROBAN_ALLOW_ZERO_GOV_TIMELOCK=1."; \ | |
| exit 1; \ | |
| fi; \ | |
| timelock_ns="${SOROBAN_GOV_TIMELOCK_NS:-}"; \ | |
| if [ -z "$timelock_ns" ]; then \ | |
| echo "Error: set SOROBAN_GOV_TIMELOCK_NS before deploying governance."; \ | |
| echo "For ephemeral local testing only, set SOROBAN_GOV_TIMELOCK_NS=0 and SOROBAN_ALLOW_ZERO_GOV_TIMELOCK=1."; \ | |
| exit 1; \ | |
| fi; \ | |
| if ! printf '%s' "$timelock_ns" | grep -Eq '^[0-9]+$'; then \ | |
| echo "Error: SOROBAN_GOV_TIMELOCK_NS must be an unsigned integer (nanoseconds)."; \ | |
| exit 1; \ | |
| fi; \ | |
| if [ "$timelock_ns" = "0" ] && [ "${SOROBAN_ALLOW_ZERO_GOV_TIMELOCK:-0}" != "1" ]; then \ | |
| echo "Error: zero governance timelock requires SOROBAN_ALLOW_ZERO_GOV_TIMELOCK=1."; \ | |
| exit 1; \ | |
| fi; \ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/vault/soroban/justfile` around lines 388 - 397, The current justfile
deploy snippet accepts non-numeric SOROBAN_GOV_TIMELOCK_NS and fails later; add
a numeric validation step after reading timelock_ns to ensure it is a
non-negative integer (e.g. use a regex like '^[0-9]+$' or a case statement) and
if the value is non-numeric print an explanatory error mentioning
SOROBAN_GOV_TIMELOCK_NS and SOROBAN_ALLOW_ZERO_GOV_TIMELOCK and exit 1; update
the block that sets timelock_ns and checks for empty/zero to first test
numericness (then continue with the existing zero-check that relies on
SOROBAN_ALLOW_ZERO_GOV_TIMELOCK).
| | I6 | Curator → `allocate_withdraw` | Yes | `require_auth(caller)` + RBAC Allocator | Vault ↔ Adapter, Vault ↔ Asset Token | | ||
| | I7 | Curator → `refresh_markets` | Yes | `require_auth(caller)` + RBAC Allocator | Vault ↔ Kernel state machine | | ||
| | I8 | Governance → `execute_governance(payload)` (fees, curator, governance, sentinel, guardian, caps, market removal, cap groups, restrictions, adapter allowlist, supply queue, skim recipient, skim, pause) | Yes | `require_auth(caller)` + governance check | Governance contract ↔ Vault storage | | ||
| | I8 | Governance → `execute_governance(payload)` (fees, curator, governance, sentinel, guardian, allocators, caps, market removal, cap groups, restrictions, adapter allowlist, supply queue, skim recipient, skim, pause) | Yes | `require_auth(caller)` + governance check | Governance contract ↔ Vault storage | |
There was a problem hiding this comment.
I8 auth description is stale after the sentinel split.
Line 120 still says “governance check,” but pause/tightening now include sentinel-authorized paths. Update the auth column so the threat model reflects the real privileged surface.
As per coding guidelines, “If behavior changes around governance bridging… update the docs in contract/vault/soroban/ in the same change.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contract/vault/soroban/STRIDE.md` at line 120, Update the I8 table row for
execute_governance(payload) to reflect the sentinel split: change the auth
column from just “require_auth(caller) + governance check” to explicitly state
both governance and sentinel-authorized paths (e.g., “require_auth(caller) +
governance OR sentinel-authorized checks (pause/tighten paths)”), and ensure the
description mentions which actions (pause, tightening, etc.) can be invoked via
sentinel authorization so the privileged surface accurately reflects
execute_governance(payload) and related governance/sentinel behavior.
…05-b407-041e82cac5ea) Trace: 488efb4
…b3f-4f54-8cca-5baeee9bc238) Trace: b6ee556
…-cf4c0af1bfec) Trace: 381a450
…-ac36-494b-a619-c6f123621abd) Trace: 0aebe10
…c-4a38-9c01-cdb806b1f1a9) Trace: 843a36f
…41fc-4b53-b8b2-95168b12803d) Trace: 08bd07a
…530-a728-d980d83b8a53) Trace: 28e2470
…fc-4b53-b8b2-95168b12803d) Trace: f8bde99
carrion256
force-pushed
the
audit/governance-control-plane
branch
from
36503d8 to
be0475b
Compare
…771-41fc-4b53-b8b2-95168b12803d)
1 participant
Summary
execute_governanceauthorization so production RBAC rejects governance-as-sentinel and sentinel-as-governance flows.Audit Context
spr/refactor/vault-ergonomics/4f330057ata657f92/data/projects/contracts-audit-tracker/contracts-audit-clusters/governance-control-plane.audit.mdVerification
cargo fmt --all -- --checkcargo test -p templar-soroban-governance --lib -- --nocapture— 29 passedcargo test -p templar-soroban-runtime test_execute_governance_separates -- --nocapture— 2 passedgit diff --checksize-budget-checkpassed at 95798 bytesHalborn Finding IDs
Included for Halborn SSC GitHub remediation detection:
e1bab0f3-3014-4905-b407-041e82cac5ea— Governance contract cannot pause, unpause, or update restrictions on deployed RBAC2e538a21-4b3f-4f54-8cca-5baeee9bc238— Default deployment recipe initializes all timelocks to zero15c2295b-5098-4025-9299-cf4c0af1bfec— Skim is immediate and default recipient is governance contractdf8f422b-ac36-494b-a619-c6f123621abd— Shipped governance contract cannot manage allocators or adapter allowlistd9db8534-9bac-4a38-9c01-cdb806b1f1a9— Contract-governance deployments have no routed path for upgrade, migrate, or cancel migrationFolded audit scope
This PR now also folds the governance ABI validation follow-ups:
Folded source PRs superseded by this PR:
audit/governance-abi-validation-a056)audit/governance-abi-validation-a066)Additional local verification after folding:
RUSTUP_TOOLCHAIN=1.89.0 cargo fmt --all --checkgit diff --checkRUSTUP_TOOLCHAIN=1.89.0 cargo test -p templar-soroban-runtime --lib -- --nocapture— 109 passedRUSTUP_TOOLCHAIN=1.89.0 cargo test -p templar-soroban-governance -- --nocapture— 36 passedjust -f contract/vault/soroban/justfile buildjust -f contract/vault/soroban/justfile size-budget-check— 96089 bytes <= 131072This change is