Skip to content

Add PMFI pARBITRAGE Bankr skill#465

Open
zaratustrastar wants to merge 2 commits into
BankrBot:mainfrom
zaratustrastar:add-pmfi-parbitrage
Open

Add PMFI pARBITRAGE Bankr skill#465
zaratustrastar wants to merge 2 commits into
BankrBot:mainfrom
zaratustrastar:add-pmfi-parbitrage

Conversation

@zaratustrastar

Copy link
Copy Markdown

Summary

Adds the PMFI pARBITRAGE Bankr skill.

Core UX:

Deposit USDC -> PMFI processes after report -> user receives pARB

Withdraw pARB -> PMFI processes after report -> user receives USDC

Commands

  • deposit <USDC>
  • withdraw <pARB>

Live contracts

Vault:

0xd1ccbc2aa6e2f41817b62448089d4125e62df4fb

Base USDC:

0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913

Tested

  • deposit dry-run
  • withdraw dry-run
  • default Base RPC fallback
  • real requestDeposit through Bankr
  • real requestRedeem through Bankr

Deposit tx:

https://basescan.org/tx/0x89918ee7f4ff63fd0cfa3581c67aa28d8bafaacbd420c329eafd5c27e45529d4

Withdraw tx:

https://basescan.org/tx/0x5e8bf069d23fc90016d35d33d2375a3894f137b6a5832e9eaee89eaa2513eae1

@saltoriousSIG saltoriousSIG left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good overall, but a couple of security issues to address before merging

  • BANKR_API_URL can leak the bankr api key.
    issue: the script sends the user’s X-API-Key to whatever url is set there, so a prompt injection could point it at an attacker server.
    fix: remove the override for normal use, or only allow trusted bankr domains.

  • PMFI_PARBITRAGE_VAULT can redirect approvals/funds.
    issue: this env var controls both the usdc approval spender and the vault call target. if changed, the script could approve/call a malicious contract.
    fix: hard-code the reviewed vault address for normal execution. if overrides are needed, gate them behind an explicit unsafe/dev mode.

  • needs prompt injection guardrails.
    issue: the skill doesn’t clearly say to ignore untrusted instructions from webpages, token metadata, tx data, rpc responses, pasted commands, etc.
    fix: add guidance that only the user’s direct request can authorize txs, and external content can’t override endpoints, vaults, receivers, amounts, or tx targets.

  • contract/admin risks are under-disclosed.
    issue: this is async, liquidity-dependent, admin-controlled, can be paused/shutdown, has emergency withdrawal functionality, and doesn’t appear to have a submitted audit.
    fix: add a clear risk note before execution so users understand the custody/admin/liquidity/timing risks.

  • missing preflight checks.
    issue: the script checks balances but not things like paused, shutdown, on-chain minimums/caps, or previewed deposit/redeem output.
    fix: read those values first and show expected shares/assets + vault state before submitting approval/deposit/redeem txs.

@zaratustrastar

Copy link
Copy Markdown
Author

thanks for the detailed review! addressed all points in the latest commit:

  • removed the BANKR_API_URL override and hard-coded the trusted Bankr API endpoint
  • removed the PMFI_PARBITRAGE_VAULT override and hard-coded the reviewed vault and Base USDC addresses
  • added endpoint, transaction-target and function-selector allowlists
  • added prompt-injection guardrails so only the user’s direct request and confirmation can authorize transactions
  • added clear async, liquidity, admin, pause/shutdown, emergency-function, custody, smart-contract, operational, strategy and audit-status risk disclosures
  • added preflight checks for vault state, on-chain minimum, cap, configured asset, balances and expected deposit/redeem output
  • added exact-call simulation and explicit user confirmation before execution

deposit and withdrawal preflights now pass, withdrawal simulation passes, and execution without direct confirmation is blocked.

would appreciate another review when you have time.

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.

2 participants