Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] [fix]: Address UMIP-179 feedback #602

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
+18 −10
Draft

[WIP] [fix]: Address UMIP-179 feedback #602

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
867733d
[fix]: Address UMIP-179 feedback
pxrl Apr 22, 2024
a45310c
Fix repayment chain wording
pxrl Apr 22, 2024
d06ad9e
Fix 1 -> 1e18
pxrl Apr 22, 2024
4812cc3
Clarify LP fee precision
pxrl Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions UMIPs/umip-179.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,11 @@ For a validated `FilledV3Relay` event, the relayer repayment amount shall be com
- `(inputAmount * (1 - realizedLpFeePct)) / 1e18`, where `realizedLpFeePct` is computed over the set of HubPool `l1Token`, `originChainId` and `repaymentChainId` at the HubPool block number corresponding to the relevant `V3FundsDeposited` `quoteTimestamp`.
- The applicable rate model shall be sourced from the AcrossConfigStore contract for the relevant `l1Token`.

If a validated `FilledV3Relay` event specifies an invalid `repaymentChainId`, the proposer shall issue repayment on the fill destination chain.
Reasons for the `repaymentChainId` to be considered invalid are:
- `repaymentChainId` is not supported by Across.
- `inputToken` is not supported as a repayment token on `repaymentChainId`.
Comment on lines +260 to +263
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think forcing repayment on the destination chain is the only safe approach here, since if the relayer fills via a contract on zkSync and the proposer proposes repayment on some other chain then the relayer will never receive their repayment.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good point. I think that's right.


### Computing Deposit Refunds
For an expired `V3FundsDeposited` event, the depositor refund amount shall be computed as `inputAmount` units of `inputToken`.

Expand Down Expand Up @@ -368,7 +373,7 @@ Each Relayer Refund Leaf shall be constructed as follows:
1. `refundAmount` descending order, then
2. `relayerAddress` ascending order (in case of duplicate `refundAmount` values).

In the event that the number of refunds contained within a Relayer Refund leaf should exceed [`MAX_RELAYER_REPAYMENT_LEAF_SIZE`]((https://github.com/UMAprotocol/UMIPs/blob/7b1a046098d3e2583abd0372c5e9c6003b46ad92/UMIPs/umip-157.md#global-constants) refunds:
In the event that the number of refunds contained within a Relayer Refund leaf should exceed [`MAX_RELAYER_REPAYMENT_LEAF_SIZE`](https://github.com/UMAprotocol/UMIPs/blob/7b1a046098d3e2583abd0372c5e9c6003b46ad92/UMIPs/umip-157.md#global-constants) refunds:
1. Additional `RelayerRefundLeaf` instances shall be produced to accomodate the excess.
2. The ordering of `refundAddresses` and `refundAmounts` shall be maintained across the ordered array of leaves.
3. Only the first leaf for a given `l2TokenAddress` shall contain a non-zero `amountToReturn`.
Expand All @@ -380,6 +385,12 @@ The set of relayer refund leaves shall be ordered according to:

The Relayer Refund Leaf `leafId` field shall be numbered according to the ordering established above, starting at 0.

If a Relayer Refund Leaf would be unable to be executed due to an ERC20 reversion when transferring the repayment token to the recipient on the destination chain, the proposer may exclude the relayer repayment from the Relayer Refund Root.
Copy link
Member

Choose a reason for hiding this comment

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

Two things:

  • Long term, I think this would be better handled in the contracts (having a data structure to store unexecuted payments that can be claimed anytime).
  • I think the way this is written, it's ambiguous. I think we should say to be excluded from the bundle, it needs to be unexecutable at the bundle end block of the chain where the repayment will take place.

Note:
- This is intended to deal with unlikely situations, such as a centralized token issuer censoring transactions concerning a relayer address, and is required to prevent deadlocking of all other relayer repayments in the same Relayer Refund Leaf.
- In the event of relayer repayment exclusion, the proposer should provide reproducible evidence that the relayer repayment would fail as part of any subsequent dispute.


Note:
- Once these leaves are constructed, they can be used to form a merkle root as described in the previous section.

Expand Down Expand Up @@ -413,7 +424,4 @@ The V3 rules defined in this UMIP will apply beginning when the VERSION field in
The Across v3 implementation is available in the Across [contracts-v2](https://github.com/across-protocol/contracts-v2) repository.

# Security considerations
Across v3 has been audited by OpenZeppelin.

Note:
- If a particular relayer refund is known to be unexecutable, it can be removed from the bundle by the proposer if a sufficient public justification is made before the proposal. This is intended to deal with unlikely situations, such as ag centralized token issuer blacklisting an address that is due a refund. If this leaf were to remain unaltered, this blacklisted address could block other addresses from recieving refunds.
Across v3 has been [audited by OpenZeppelin](https://blog.openzeppelin.com/across-v3-incremental-audit).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Across v3 has been [audited by OpenZeppelin](https://blog.openzeppelin.com/across-v3-incremental-audit).
Across v3 has been audited by OpenZeppelin, shared [here](https://blog.openzeppelin.com/across-v3-incremental-audit) and [here](https://blog.openzeppelin.com/across-v3-and-oval-incremental-audit).