fix(blockchain-api): allow an off-curve owner when deriving associated token addresses - #1232
Conversation
bryzettler
left a comment
There was a problem hiding this comment.
Review of the off-curve/Squads changes. Line comments below cover what's anchorable in the diff; two issues live in files this PR doesn't touch, so they go here:
1. splitPosition bypasses the new ephemeral-signer guard (governance/procedures/positions/split.ts ~L160)
split.ts builds directly via buildVersionedTransaction({ ..., signers: [newMintKeypair] }) with no canSign check and no packGroups, so an off-curve (Squads-vault) wallet gets a mint-keypair-signed transaction back. Re-wrapping it into vault_transaction_create (ephemeralSigners: 0) keeps the message but drops the mint signature — exactly the failure mode the guard in pack-groups.ts refuses: members approve, execution fails on-chain after quorum and rent are spent. The invariant ("refuse rather than hand back a proposal whose execution can only fail") currently only covers the batched path; split should hit the same refusal.
2. Fee gates still charge jitoTipCost for payers that no longer get a tip
The estimated-fee/INSUFFICIENT_FUNDS gates add jitoTipCost whenever versionedTransactions.length > 1 on mainnet, but with this PR an off-curve payer never receives a tip transaction. A vault whose balance sits between the true requirement and requirement + tip gets a spurious INSUFFICIENT_FUNDS, and every off-curve caller sees an overstated estimate. The condition should mirror the tip decision (canSign(feePayer) && shouldUseJitoBundle(...)). Affected: delegation/claim-rewards.ts (~L126), delegation/delegate.ts (~L272/L622), delegation/undelegate.ts, positions/create.ts (~L373), proxy/assign.ts (~L360), relinquish-position-votes.ts.
…d token addresses A Squads vault owns its assets through a program address, which is off the ed25519 curve. Eleven call sites derived associated token addresses without `allowOwnerOffCurve`, so `getAssociatedTokenAddressSync` threw `TokenOwnerOffCurveError` and failed the whole request rather than one derivation. `buildClaimInstructions` is one of them, and the claim, undelegate and delegate procedures all call it first, so claiming delegation rewards, undelegating and re-delegating an expired position were unavailable to every vault-held veHNT position with an unclaimed epoch. `delegate.ts` derived the reward account the same way when enabling autoclaim, so a vault could not turn automation back on either. The derived address is identical either way and the programs re-validate the account they receive against their own `associated_token::authority` and `token::authority` constraints, so this decides whether a multisig can use an endpoint at all, not who is allowed to do what. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b60fc17 to
0a305f2
Compare
|
Cut this back to the associated-token fix only; the packing and Jito changes are gone ( The packing half was dropped because its benefit is blocked downstream: That resolves the review comments on Two comments survive as follow-ups recorded in the description: the |
…at cannot sign Both build a transaction signed by an ephemeral mint keypair. A wallet that cannot sign is served by re-wrapping the message into a multisig proposal, which keeps the message and discards that signature, so the proposal can only fail on execution once quorum and rent are spent. `createPosition` was previously stopped by accident: its unconditional deposit-balance check derived an associated token address that threw for an off-curve owner, so the request failed before returning anything. Allowing an off-curve owner there removes that accidental stop, which makes the unexecutable path reachable, so the refusal is now explicit and typed. `splitPosition` never had the accidental stop and was reachable already. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Status of each review comment, after cutting the packing half and adding the typed refusal. Addressed
Resolved by removing the code they applied to (verified:
Not addressed, recorded in the description
Coverage honesty: the only new test is on |
Problem
A Squads vault owns its assets through a program address, which is off the ed25519 curve. Eleven call sites derived associated token addresses without
allowOwnerOffCurve, sogetAssociatedTokenAddressSyncthrewTokenOwnerOffCurveErrorand took down the whole request rather than one derivation. Reproduced against a mainnet vault-held position by invoking the real helper:buildClaimInstructionsis one of those call sites, and claim, undelegate and delegate all call it first, so all three were unavailable to any vault-held veHNT position with an unclaimed epoch.delegate.tsderived the reward account the same way when enabling autoclaim, so a vault could not turn automation back on either.The derived address is identical either way and the programs re-validate the account they receive (
claim_rewards_v1constrainsdelegator_atabyassociated_token::authorityandposition_token_accountbytoken::mintplus a positive balance), so this decides whether a multisig can use an endpoint at all, not who is allowed to do what.Scope
This started larger and was cut back deliberately. It also changed transaction packing so a vault's batches would survive being re-wrapped as a Squads proposal, and skipped the Jito tip for a payer that cannot sign. Both are dropped, because the benefit turned out to be blocked downstream:
getJitoTipTransactionbuilds aSystemProgram.transferfrom the fee payer, so a vault can never sign a tip.JitoMissingTipErrorwhen no tip is present.So a multi-transaction batch for a vault fails at submission with or without the packing change, and a smaller size budget buys nothing until that is fixed. Everything else in this PR is a plain correctness fix, so it ships on its own.
Known follow-ups, not in this PR
ata(mint, owner)wrapper that always passes it, plus an ESLint restriction on importinggetAssociatedTokenAddressSyncdirectly, which fixes the class rather than scanning for instances. That means migrating all 30 existing call sites, which does not belong in a fix this size.positions/create.tsandpositions/split.tsattach an ephemeral mint keypair. A wrapping wallet keeps the message and drops that signature, so a vault gets a proposal whose execution can only fail. These endpoints should reject an off-curve fee payer with a typed error.jitoTipCostis added whenever there is more than one transaction on mainnet.squadsProposeFieldsis spread into the token, data-credit and hotspot schemas only; no governance procedure readsinput.multisigor callsbuildActionProposal.buildActionProposalis also single-proposal, so a delegation recovery spanning many transactions would need a multi-proposal variant.pnpm test:unitis referenced by no workflow, so the four existing unit tests in this package never run in CI.