fix: separate refund claim when there is no token account #633

Reinis-FRP · 2024-10-07T15:35:06Z

This fix protects against a relayer passing invalid refund token account to block the whole relayer leaf execution. If a relayer had provided invalid ATA for refunds, the leaf executor can replace it with a claim account PDA that is derived from mint and relayer's token account to only accrue given relayer's refund claims. If this was a valid token address, but was closed at the time of leaf execution, all accrued claims can be refunded separately whenever the given refund account is created.

For the sake of code simplicity, this does not attempt to initialize a claim account when executing the relayer refund leaf. The operator is expected to initialize it in advance, but this is required only when a given relayer's token account is invalid.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

chrismaree · 2024-10-16T11:17:29Z

programs/svm-spoke/src/state/refund_account.rs

+    pub amount: u64,
+}
+
+// When executing relayer refund leaf, refund accounts are passed as remaining accounts and can hold either a regular


does this mean that, in theory, a relayer could close their ATA right before the bundle execution tx lands on-chain, thereby making the execution of the bundle in-valid as the remaining account type is wrong? so when the executor looks at the tx to send they build TokenAccount, expecting that the relayer has an ATA but before the tx lands on-chain the relayer can front run the tx and delete their ATA, which then requires, for this method to work, that the executor passes in the ClaimAccount, not token account, which breaks the bundle execution and the whole thing reverts?

In theory, this means that a relayer who can front run the execution can always "toggle" the status of their ATA (opening it or closing it), thereby blocking the bundle execution?

if the executor sends these execution transactions via private mempool they might be able to protect themselves (you cant close/open the account before the execution) but this is not going to be failsafe?

thoughts on this?

In theory that's possible, but might be a bigger effort as there is no public mempool, so the attacker has to run Solana validator co-located close to our bot. We can reduce attack surface by sending the transaction to trusted validator. And if this is still an issue, we can fall-back using claim accounts after couple of reverts.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

chrismaree · 2024-10-18T14:14:54Z

programs/svm-spoke/src/instructions/bundle.rs

+where
+    'c: 'info,
+{


can you help me understand this bit of syntax a bit better? I also see it in the lib.rs.

Added the constraint that lifetime for remaining accounts ('c) is at least as long as 'info for accounts that are referenced by this account array. I think its needed because of manual remaining account processing in try_from_remaining_account method. Without these lifetime constraints the compiler errors.

o that's interesting. ye, I've had other lifetime issues while implementing other things and was able to address it through other syntax but this is more complex here.

chrismaree · 2024-10-18T14:16:39Z

programs/svm-spoke/src/state/refund_account.rs

@@ -0,0 +1,132 @@
+use anchor_lang::prelude::*;


nit on this file naming: we call the instruction refund_claims and the instruction refund_accounts. we should think about it makes sense to re-name this state to claim_account? equally, for other files between instruction & state having a consistent name between the two.

RefundAccount can hold either TokenAccount or ClaimAccount and state/refund_account.rs has methods for handling both, so would not want to name it as claim_account. Also the wrapped RefundAccount is used in bundle instruction while individual ClaimAccount is used in refund_claims instruction, so there is no 1-1 relationship between state and instructions like we have in other places.

chrismaree · 2024-10-18T14:17:42Z

programs/svm-spoke/src/state/refund_account.rs

+    // #[account(
+    //     mut,
+    //     seeds = [b"claim_account", mint.key().as_ref(), token_account.key().as_ref()],
+    //     bump
+    // )]
+    // pub claim_account: Account<'info, ClaimAccount>,


is this meant to be commented out?

Yes, it is meant to document what Anchor macros this method is reimplementing, so its easier to audit knowing the intention of the implementation.

chrismaree · 2024-10-18T14:20:58Z

test/svm/SvmSpoke.Bundle.ts

+  describe("Execute Max Refunds", () => {
+    const executeMaxRefunds = async (refundType: RefundType) => {
+      const relayerRefundLeaves: RelayerRefundLeafType[] = [];
+      // Higher refund count hits inner instruction size limit when doing `emit_cpi` on public devnet. On localnet this is


to test this on testnet did you use the script?

it would be wise I think for us to run this same test on mainnet to ensure that there are no other memory issues that we don't know about on this. does mean deploying the whole program, but that's ok as we can always close it afterwards.

Yes, here is the refund execution for 28 relayers. At 29 count would get this error: Instruction passed to inner instruction is too large (1306 > 1280) when doing emit_cpi.

Though, we might need to improve the script as in some runs it closed the instruction parameter account before leaf execution tx got mined.

chrismaree · 2024-10-18T14:23:18Z

test/svm/SvmSpoke.Bundle.ts

+      });
+    };
+
+    it("Execute Max Refunds to Token Accounts", async () => {


this is cool. nice structure within the describe. these tests are now...complex, but that's ok.

chrismaree · 2024-10-18T14:25:46Z

programs/svm-spoke/src/state/refund_account.rs

+    // #[account(
+    //     mut,
+    //     address = expected_token_account @ CustomError::InvalidRefund,
+    //     token::mint = expected_mint,
+    //     token::token_program = token_program
+    // )]
+    // pub token_account: InterfaceAccount<'info, TokenAccount>,


is this mean to be commented out?

yes, left them for auditing purposes as we reimplement what Anchor macro would have done on static account.

chrismaree · 2024-10-18T14:46:13Z

programs/svm-spoke/src/instructions/bundle.rs

+    // Emit the DeferredRelayerRefunds event if any claim accounts were passed instead of token accounts. We cannot emit
+    // individual accounts and amounts as that would run out of memory for larger leaves.
+    if deferred_refunds {
+        emit_cpi!(DeferredRelayerRefunds {


I think it's ok to emit all this but it's not really needed, right? we really just want to know that it happened, but care less about all the other props give we have ExecutedRelayerRefundRoot.

an alternative structure could simply be to add a bool to ExecutedRelayerRefundRoot that indicates if deferred_refunds is set so we don't need a. new event?

yes, if we can change interface of ExecutedRelayerRefundRoot this would be much easier to avoid separate emit_cpi

pushed the commit to include the bool in ExecutedRelayerRefundRoot

chrismaree · 2024-10-18T14:49:21Z

programs/svm-spoke/src/state/refund_account.rs

+        }
+
+        // Checks if the token account is associated with the expected mint.
+        if &token_account.mint != expected_mint {


this is a meta-comment but this check has a & (borrowing) and the others don't. expected_token_account is passed in & but is not checked against the account_info with borrowing.

point is: is there away to do this more consistently in this file and beyond?

not sure. owner and key in AccountInfo are borrowed references, while InterfaceAccount::try_from returns token_account with all of its properties owned. That's why we need to handle them differently. And since compiler automatically dereferences I think its more common pattern not to do explicit dereferencing, instead pass borrowed variables to comparison and let the compiler dereference them where needed.

but you're right that its not fully consistent with comparison in try_claim_account_from_account_info below, so I just pushed commit to use account_info.key != &pda_address instead of account_info.key() != pda_address on line 121.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Reinis-FRP added 7 commits October 7, 2024 15:29

fix: separate refund claim when no token account

64103df

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix

fdf2201

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Merge branch 'master' into reinis-frp/refund-claims

3deb865

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix

168dab2

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: do not log debug errors

e1be75d

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: remove redundant error code

120c198

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix

1f31c01

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Reinis-FRP marked this pull request as ready for review October 8, 2024 13:14

Reinis-FRP requested review from chrismaree and md0x October 8, 2024 13:14

chrismaree reviewed Oct 16, 2024

View reviewed changes

Reinis-FRP added 9 commits October 16, 2024 13:56

Merge branch 'master' into reinis-frp/refund-claims

79fa785

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

test: max refunds on claim accounts

4c0fb22

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

test: claim account

397885a

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: use if instead of require in zero refund claim

2361d31

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: close claim account and emit event on claiming

ec8e213

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

test: separate refund claim tests

fdbd8d4

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Merge branch 'master' into reinis-frp/refund-claims

d6f5e15

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: emit event when any refunds are deferred

e904f90

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix

c7758ec

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Reinis-FRP requested a review from chrismaree October 18, 2024 10:20

fix: linting

79476ba

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

chrismaree reviewed Oct 18, 2024

View reviewed changes

Reinis-FRP added 2 commits October 18, 2024 15:41

fix: consistent comparison

cdb9ac3

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Merge branch 'master' into reinis-frp/refund-claims

d1caae6

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

Reinis-FRP requested a review from chrismaree October 18, 2024 15:53

Reinis-FRP added 2 commits October 21, 2024 06:55

fix: merge event data

655219d

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

fix: remove reduntant event

bb1f745

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

chrismaree approved these changes Oct 21, 2024

View reviewed changes

Reinis-FRP merged commit 503332f into master Oct 21, 2024
8 of 9 checks passed

Reinis-FRP deleted the reinis-frp/refund-claims branch October 21, 2024 08:52

fix: separate refund claim when there is no token account #633

fix: separate refund claim when there is no token account #633

Uh oh!

Conversation

Reinis-FRP commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Reinis-FRP commented Oct 7, 2024 •

edited

Loading