fix: separate instruction for deferred relayer refunds #715
Conversation
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
| pub mint_public_key: Pubkey, | ||
| #[max_len(0)] | ||
| pub refund_accounts: Vec<Pubkey>, | ||
| pub refund_addresses: Vec<Pubkey>, |
There was a problem hiding this comment.
nice. This is clearer now, given the address of the owner of the ATA. it will be useful to add a comment to this effect to distinguish between the kinds of props being passed in within the leaf. (at minimum a TODO to call out the need to comment this effectively when we write comments)
|
|
||
| // Token address has been checked when executing the relayer refund leaf and it is part of claim account derivation. | ||
| #[account(mut, token::mint = mint, token::token_program = token_program)] | ||
| // TODO: In EVM we currently allow claiming refunds to any address, but probably should restrict it there as well. |
There was a problem hiding this comment.
well I think that's a feature, no? an alternative implementation that we could structure here, given we are using addresses (owner) rather than token_account (ATA/TA) is to simply require that the signer is the set refund_address and then let the caller send the funds to any recipient TA they want.
The reason it was done in the EVM is that if you set the refund address to a blocked address then you're assets are stuck forever unless you can send them somewhere else. Under the assumption that blacklists are implemented universally and consistently, this feature does not let you bi-pass blacklists or anything like that as you would not be able to deposit in the first place. rather, it provides a clean recovery flow, assuming you can prove ownership of the recipient address.
point being: we should be consistent here and could consider doing the same change to simply verify signer and then let you choose which TA to send things to.
There was a problem hiding this comment.
If claim can be made by anyone on behalf of the relayer then this still better be canonical ATA as otherwise it would be cumbersome for the relayer to reconcile their refunds.
Though, we could create an additional flavor to this method where only the relayer can call it and claim to any token account they control?
There was a problem hiding this comment.
ye, the latter is what I'm suggesting. only the relayer can claim it, assuming they are the signer which matches with the original repayment recipient in the original fill. this tracks with the EVM implementation more directly: https://github.com/across-protocol/contracts/pull/672/files#r1827795557
There was a problem hiding this comment.
but this does relate to the point in the other comment thread about enabling others to execute the claim on behalf of the relayer if the transfer is possible. the additional flavour might then be the best of both worlds:
- anyone can execute the claim on behalf of a relayer wherein they create the ATA and we send the tokens to the ATA for the claim. the claim is then tracking the outstanding amount for the address (the owner of the ATA).
- if the above fails due to blacklist, then the owner of the address (signer in this extra flavour) can call to send the tokens to a new ATA. clearly, only if they can prove they are the address (singer) can they do this.
This way, the first flow we can use to auto re-fund relayers if we need to use claim accounts and if relayers get stuck they can still recover their funds on theirown
|
|
||
| pub fn execute_relayer_refund_leaf<'c, 'info>( | ||
| ctx: Context<'_, '_, 'c, 'info, ExecuteRelayerRefundLeaf<'info>>, | ||
| deferred_refunds: bool, |
There was a problem hiding this comment.
so now the caller of this method chooses to defer the refunds, or not. in theory this lets people front run the executor bot who would be by default (happy path) setting this to false, and then they execute it setting it to true, forcing relayers to get the deferred refund.
Is there a way to programmatically detect this? my guess is that it will be difficult as we'd basically need to try distribute_relayer_refunds and only if that fails then accrue_relayer_refunds, which is not really a possible flow as by that point we might have sent some tokens already. this means that we'd somehow need to know before hand that the whole bundle is executable via distribute_relayer_refunds before sending any assets.
I guess this is why you structured it like this?
There was a problem hiding this comment.
Also in the prior implementation the caller implicitly could chose defer refunds by passing all claim accounts. But I agree, this still might be an issue and we could require that the caller always passed all correct mutable ATAs (even if closed or blacklisted) and in the happy path try to transfer all of them. Only if any transfer fails we require that caller had also passed additional claim accounts and defer the claims only for those refunds that did not go through.
Initially I was bit hesitant to do this as that could require doubling the remaining accounts, but its possible to avoid this in the happy path and we are using ALTs anyway. So I will try to do this now.
There was a problem hiding this comment.
ok cool. otherwise I think the way you have it now is ok, btw, it's just a path we need to know about.
There was a problem hiding this comment.
Unfortunately, I overlooked the fact that CPIs in Solana halt tx execution immediately, so its not really possible to handle failed transfers by accruing the claim and proceeding to the next refund. So will need to leave this as 2 separate execute relayer refund flavors: one with all ATAs and another with all claim accounts.
As for the original concern, there is not a really good way to protect against frontrunning. Though, we can still claim relayer refunds automatically for all relayers if we detect this.
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
| #[account( | ||
| mut, | ||
| associated_token::mint = mint, | ||
| associated_token::authority = refund_address, |
There was a problem hiding this comment.
Anchor checks both that ATA is derived from refund_address and that it is owned by refund_address. This differs slightly from the implementation in execute_relayer_refund_leaf where we only check ATA address (as owner might have transferred authority), but should not be a big issue as the relayer can recover by calling claim_relayer_refund to any custom token account.
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
| let mut deferred_refunds = false; | ||
| // Check if vault has sufficient balance for all the refunds. | ||
| let total_refund_amount: u64 = relayer_refund_leaf.refund_amounts.iter().sum(); | ||
| if ctx.accounts.vault.amount < total_refund_amount { |
There was a problem hiding this comment.
Question: Would it make sense to consider the hub pool liabilities in the amount available to transfer?
There was a problem hiding this comment.
Was considering to add this amount initially, but that could still cause race to claim if vault balance decreased due to slow fills or claims from prior leaf executions. So the check here is more for the sake of consistency with EVM, but its effectiveness is bit limited.
In order simplify the implementation of relayer refund execution, this allows either all refund accounts being token accounts or all being claim accounts and separates the instruction in two flavors.
In addition, this further restricts refund token accounts to canonical ATAs as derived from relayer authority addresses. Before the leaf included refund token accounts, but that did not allow the operator to create ATA if it is inactive. Now the relay leaf would include relayer authority addresses and ATAs can be created by anyone to unblock leaf execution.
This includes a new test case where ATAs creation and leaf execution are packed in one transaction that shows we can do this for up to 9 relayer refunds.
fixes: https://linear.app/uma/issue/ACX-3025/staterefund-accountrs-consider-if-we-can-avoid-this-dual-account-and