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.

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

feat(protocol): add bridge rate limiter for ETH and ERC20s #16970

Merged
merged 42 commits into from
May 6, 2024
Merged

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented May 3, 2024

Following open zeppelin's suggestion, this PR introduces a feature to manage cross-chain quota. For each token, we can config the daily quota.

Note that the quota is only consumed and checked on the destination chain (process message and retry message) and on the source chain (recall message but NOT send message). So if I set 1000 ether daily quota on L1, on L2, people can still withdraw more than 1000 ether, but on L1, they can NOT withdraw more than 1000 ether per day, so bridge and UI doesn't need to add new features to check available quota when messages are sent.

The relayer backend does need to check the quota before processing messages, otherwise, the relayer will not get paid.

Lets vote on this. I'm ok with or without this PR to be merged.

  • thumb up for yes
  • thumb down for no

@dantaik dantaik changed the title feat(protocol: add bridge rate limiter for ETH and ERC20s feat(protocol): add bridge rate limiter for ETH and ERC20s May 3, 2024
Copy link

openzeppelin-code bot commented May 3, 2024

feat(protocol): add bridge rate limiter for ETH and ERC20s

Generated at commit: 36d6573b0aa2a3f343c3b1000c7e6f1b978ecf5b

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
5
41
50
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@dantaik
Copy link
Contributor Author

dantaik commented May 3, 2024

Need to add some tests later

@dantaik dantaik marked this pull request as ready for review May 3, 2024 06:10
@dantaik dantaik changed the title feat(protocol): add bridge rate limiter for ETH and ERC20s feat(protocol): add bridge rate limiter for ETH and ERC20s (should we merge it?) May 3, 2024
@dantaik dantaik requested a review from adaki2004 May 3, 2024 14:30
packages/protocol/script/DeployL1QuotaManager.s.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/bridge/QuotaManager.sol Outdated Show resolved Hide resolved
@dantaik dantaik requested a review from Brechtpd May 4, 2024 03:34
@cyberhorsey
Copy link
Contributor

cyberhorsey commented May 5, 2024

@dantaik given the relayer sends multiple messages at a time (ie: it has multiple pending transactions simultaneously), what would be the best way to ensure we don't waste gas on reverted transactions, given this new rate limit cap?

Is there also a method or parameter we can add to get when the issuance is reset, so the relayer knows how long to wait? It could be calculated now using off chain code duplication now, but maybe easier to just add a member onchain to call.

@Brechtpd
Copy link
Contributor

Brechtpd commented May 5, 2024

@dantaik given the relayer sends multiple messages at a time (ie: it has multiple pending transactions simultaneously), what would be the best way to ensure we don't waste gas on reverted transactions, given this new rate limit cap?

Not specifically for this but in general the relayer will have the same problem as people proposing blocks in a based rollup: So I will again shill hooking into PBS also for this like with https://docs.flashbots.net/flashbots-protect/overview (though I don't know what the best service is to do this in practice). There is no other way (except maybe some of the account abstraction stuff for revert protection but I don't think that's directly usable now and maybe also not sufficient) to prevent both other parties stealing transactions and also ensuring no money is wasted on failed/reverted transactions.

Based rollups can't exist without hooking into PBS for the same reason. The problem here where messages can be processed by anyone is the same, though here keeping the transaction private is less of a concern but you still need to have protection against failed transactions so have to make the L1 tx fee payment conditional on the successful execution somehow. Otherwise as soon as there are multiple relayers processing messages there will be (as vitalik called it) total anarchy and everybody losing money on failed transactions.

@cyberhorsey
Copy link
Contributor

@dantaik given the relayer sends multiple messages at a time (ie: it has multiple pending transactions simultaneously), what would be the best way to ensure we don't waste gas on reverted transactions, given this new rate limit cap?

Not specifically for this but in general the relayer will have the same problem as people proposing blocks in a based rollup: So I will again shill hooking into PBS also for this like with https://docs.flashbots.net/flashbots-protect/overview (though I don't know what the best service is to do this in practice). There is no other way (except maybe some of the account abstraction stuff for revert protection but I don't think that's directly usable now and maybe also not sufficient) to prevent both other parties stealing transactions and also ensuring no money is wasted on failed/reverted transactions.

Based rollups can't exist without hooking into PBS for the same reason. The problem here where messages can be processed by anyone is the same, though here keeping the transaction private is less of a concern but you still need to have protection against failed transactions so have to make the L1 tx fee payment conditional on the successful execution somehow. Otherwise as soon as there are multiple relayers processing messages there will be (as vitalik called it) total anarchy and everybody losing money on failed transactions.

:|

@dantaik
Copy link
Contributor Author

dantaik commented May 5, 2024

  • There is a QuotaUpdated event, it will be emitted when the quota changed by the admin.
  • availableQuota can be used to query the currently available quota, one additional parameter is added so you can query the available quota at current timestamp + x seconds

@dantaik
Copy link
Contributor Author

dantaik commented May 6, 2024

BTW, we'll deploy a QuotaManager on only L1 for Ether and USDC/USDT initially.

@cyberhorsey
Copy link
Contributor

BTW, we'll deploy a QuotaManager on only L1 for Ether and USDC/USDT initially.

so Quota manager should be optional in the relayer. Got it, will update my PR accordingly.

Co-authored-by: D <51912515+adaki2004@users.noreply.github.com>
@dantaik dantaik enabled auto-merge May 6, 2024 08:18
@dantaik dantaik added this pull request to the merge queue May 6, 2024
Merged via the queue into main with commit d048a28 May 6, 2024
3 of 4 checks passed
@dantaik dantaik deleted the rate_limiter branch May 6, 2024 08:26
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.

5 participants