Skip to content

docs(runbook): verify fee account trustline before registering tokens or rotating fee account (closes #141) - #146

Open
rushikeshgarad2024-dev wants to merge 2 commits into
boundlessfi:testnetfrom
rushikeshgarad2024-dev:docs/fee-trustline-preflight-141
Open

rushikeshgarad2024-dev wants to merge 2 commits into
boundlessfi:testnetfrom
rushikeshgarad2024-dev:docs/fee-trustline-preflight-141

Conversation

@rushikeshgarad2024-dev

Copy link
Copy Markdown

Summary of Changes

Addresses Issue #141 and threat model finding DoS.15 (unverified fee account trustlines leading to transaction reverts during protocol fee collection in deposit_with_fee_at / release_with_fee_at):

  1. New Pre-flight Verification Tool (scripts/admin/verify-fee-trustline.sh):
    • Implements automated Horizon querying to verify that candidate fee_account G-addresses exist, hold active trustlines for required/registered asset pairs, and have is_authorized: true.
  2. Pre-flight Checklist Update (docs/multisig-preflight.md):
    • Added Section 4.B specifying fee account trustline verification requirements before register_supported_token or set_fee_account operations.
  3. Operations Runbook & Event Alerting (docs/contract-ops-runbook.md):
    • Documented operational verification commands and recommended event alerting hooks for FeeAccountUpdated and TokenRegistered.
  4. Integration with scripts/admin/verify-multisig.sh:
    • Updated guidance step to cross-reference trustline pre-flight checks.

Closes #141.

…nbook (closes boundlessfi#141)

- Add `./scripts/admin/verify-fee-trustline.sh` to pre-flight verify fee account trustlines and authorization status across registered tokens (mitigating DoS.15).
- Update `docs/multisig-preflight.md` with fee account pre-flight trustline verification checklist before `register_supported_token` and `set_fee_account`.
- Update `docs/contract-ops-runbook.md` with operational trustline validation procedures and event alerting guidelines on `FeeAccountUpdated` and `TokenRegistered`.
- Link trustline check into `scripts/admin/verify-multisig.sh` next-step guidance.
Comment thread scripts/admin/verify-fee-trustline.sh Outdated

# Match either exact code+issuer or asset code
MATCH=$(echo "$BALANCES" | jq -r --arg code "$CODE" --arg issuer "$ISSUER" \
'[.[] | select(.asset_code == $code and (.asset_issuer == $issuer or $code == $issuer))] | .[0]')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium Severity severity

Business Logic: false positive issuer match in verify-fee-trustline.sh

The trustline match predicate can succeed even when the provided issuer does not match, because it includes (.asset_issuer == $issuer or $code == $issuer). If an operator supplies malformed input (e.g., missing :ISSUER) or an attacker convinces ops to verify only by ASSET_CODE, the script may incorrectly PASS a fee account that lacks the correct trustline, leaving deposit_with_fee_at / release_with_fee_at vulnerable to the same revert/DoS condition this PR aims to mitigate.

Require exact asset_code+asset_issuer matching for credit assets (and reject inputs that are not CODE:ISSUER), so the script cannot pass on code-only matches or wrong issuers.

Suggested change
'[.[] | select(.asset_code == $code and (.asset_issuer == $issuer or $code == $issuer))] | .[0]')
'[.[] | select(.asset_code == $code and .asset_issuer == $issuer)] | .[0]')

Fix with MCP
Almanax found a vulnerability. Can you take a look and fix it?
Finding ID: 8b0424b8-3a25-4529-bfae-b9d14299c424
Actions
  • Reply /almanax ask <question> to ask a follow-up question.
  • Reply /almanax dismiss [<reason>] and it won't appear again in future scans.
  • Reply /almanax resolve [<reason>] to mark the finding as resolved.
  • Reply /almanax severity <level> [<reason>] to override the severity.

Comment thread scripts/admin/verify-fee-trustline.sh Outdated
'[.[] | select(.asset_code == $code and (.asset_issuer == $issuer or $code == $issuer))] | .[0]')

if [[ -n "$MATCH" && "$MATCH" != "null" ]]; then
IS_AUTH=$(echo "$MATCH" | jq -r '.is_authorized // true')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low Severity severity

Input and Parameter Validation: non-conservative is_authorized default in verify-fee-trustline.sh

For required trustlines, the script treats missing .is_authorized as true (.is_authorized // true). If Horizon omits this field in some responses/edge cases, the script can incorrectly mark an unauthorized trustline as authorized, again undermining the operational DoS mitigation.

Default missing .is_authorized to false when asserting required trustlines, so the script fails closed and forces manual verification.

Suggested change
IS_AUTH=$(echo "$MATCH" | jq -r '.is_authorized // true')
IS_AUTH=$(echo "$MATCH" | jq -r '.is_authorized // false')

Fix with MCP
Almanax found a vulnerability. Can you take a look and fix it?
Finding ID: deb2bf4e-96f1-4373-b765-d78b568850d8
Actions
  • Reply /almanax ask <question> to ask a follow-up question.
  • Reply /almanax dismiss [<reason>] and it won't appear again in future scans.
  • Reply /almanax resolve [<reason>] to mark the finding as resolved.
  • Reply /almanax severity <level> [<reason>] to override the severity.

…d check

- Enforce exact `.asset_code == $code and .asset_issuer == $issuer` matching in `verify-fee-trustline.sh`.
- Default missing `.is_authorized` to `false` (fail-closed validation).
- Addresses review feedback from @almanax-ai[bot].
@rushikeshgarad2024-dev

Copy link
Copy Markdown
Author

Thank you for the review @almanax-ai[bot]!

Updated verify-fee-trustline.sh in the latest commit:

  1. Enforced exact .asset_code == $code and .asset_issuer == $issuer matching to eliminate false positives on credit assets.
  2. Updated .is_authorized default to fail-closed (.is_authorized // false).

/almanax resolve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Runbook: verify the fee account holds a trustline before registering a token or rotating the fee account

1 participant