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

Add EIP-5075: RateLimit, an outflow limiter for assets #5075

Closed
wants to merge 45 commits into from

Conversation

pr0toshi
Copy link

@pr0toshi pr0toshi commented May 5, 2022

Limits outflows for all contract assets to a given rate in a given timeframe to limit losses from hacks

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

Limits outflows for all contract assets to a given rate in a given timeframe to limit losses from hacks
@eth-bot
Copy link
Collaborator

eth-bot commented May 5, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-5075.md

classification
newEIPFile

EIPS/eip-transfer_rate_limit Outdated Show resolved Hide resolved
EIPS/eip-transfer_rate_limit Outdated Show resolved Hide resolved
EIPS/eip-transfer_rate_limit Outdated Show resolved Hide resolved
pr0toshi and others added 2 commits May 6, 2022 12:29
@pr0toshi
Copy link
Author

pr0toshi commented May 6, 2022

Updated license and assigned eip. Was going to at the start but the process specifically says to wait for editor. Ty for the review.

@MicahZoltu
Copy link
Contributor

The filename needs updated to EIPs/eip-5075.md.

@pr0toshi
Copy link
Author

pr0toshi commented May 7, 2022

My bad, saw needed to change just forgot when updating the rest.

Copy link

@artdgn artdgn left a comment

Choose a reason for hiding this comment

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

Cool trick to track the rate smoothly, two small concerns about the calculations:

  1. Possible underflow in the reference implementation.
  2. Rate limit in effect per time window can be exceeded by about 2X (in the example I thought of), perhaps worth to mention the approximate nature of the limit (or derive the worst possible deviation from it, which may be 2X but I'm not sure)

EIPS/EIPs/eip-5075.md Outdated Show resolved Hide resolved
EIPS/EIPs/eip-5075.md Outdated Show resolved Hide resolved
EIPS/EIPs/eip-5075.md Outdated Show resolved Hide resolved
EIPS/EIPs/eip-5075.md Outdated Show resolved Hide resolved
EIPS/EIPs/eip-5075.md Outdated Show resolved Hide resolved
@pr0toshi
Copy link
Author

pr0toshi commented May 8, 2022

@artdgn ah you are right does not strictly rate limit, more an av. The actual limit would be based on the previous 1h and should that be 0 yes approaches 2x for that 1h slot, though to be noted does not apply for following slots so rather than 2x, actually ends up

Net outflow limit for any time t can be seen as
ratelimit available time 0 + t * rateLimit not simply t * rateLimit, 2x for cases where the start would be 0 for the previous 1h (no activity last 1h).

So let's say that you have a time window at 100m and a rateLimit at 100 bips (so lets just say theres 1000 tokens and so 100 tokens per 100m)

Time (m)
Amount out (up to limit)
000 005 010 020 040 060 080 099 100 105 110 120...
100 005 010 010 020 020 020 014 001 005 005 010...
Delta 1h
100+.............................................099 (199 last 1h)
.......005+....................................................105 (105)

Amount out last 1h
100 105 115 125 145 165 185 199 100 105 105 110 ...

So realistically ends up as the excess possible being the free rate available at the time 0 for the window. Will update the proposal to say that. Would do mappings for amounts to stamps but the gas costs would be much higher. Though could be an alternate implementation for L2s.

Am on a phone atm so my b that may be hard to understand what was shown.

Need to check to confirm though

Note
Making the window and limit small for that window limits available rate at t0. So window 5m, limit 5 may be better, though having a spikey capacity that smoothes and makes the av rate approach target could be useful. (As at worst with a 100 bip, 1h window, would take an attacker 9h to extract vs 10) as t0 would be 1h worth, but they can only do up to the rate limit every 1h after.

@pr0toshi pr0toshi requested a review from MicahZoltu May 10, 2022 11:15
@pr0toshi
Copy link
Author

Have updated all needed details, detailed the reference implementation considerations, made the way that the limit be handled be a recommendation rather than a set value or way. Tested gas benchmarks aswell (about 12k overhead per transfer vs non rateLimited) updated the params to be more clear. Should be all good now.

@pr0toshi
Copy link
Author

pr0toshi commented Jul 26, 2022

Here's my first pass. I haven't looked at the Specification, Rationale, or Backwards Compatibility sections yet.

Thankyou so much for the thorough run through. Will update the names, was just trying to keep short so that thered be a signal that it's an extension of transfer and to be used as a replacement for the most part, but will look at alternatives such as .send vs. .transfer

@github-actions
Copy link

The commit 7c7fec6 (as a parent of d2936a3) contains errors. Please inspect the Run Summary for details.

EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
EIPS/eip-5075.md Outdated Show resolved Hide resolved
@github-actions
Copy link

The commit 16cbbfb (as a parent of 93dd9ba) contains errors. Please inspect the Run Summary for details.

Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

So I have to ask: why is this proposal an EIP instead of a library?

Generally speaking, EIPs are useful when you'll have multiple implementations that need to coordinate so they're all compatible. This proposal seems like you can add it to existing EIP-20 tokens without really changing the external interface.

If you strongly feel like this should be an EIP, please expand the Motivation section with your reasoning.

@eth-bot eth-bot dismissed stale reviews from ghost and lightclient via 16cbbfb August 10, 2022 17:12
@Pandapip1 Pandapip1 changed the title Create EIP5075 - Rate Limiter for Token Outflows from Contracts Add EIP-5075: Rate Limiter for Token Outflows from Contracts Sep 1, 2022
@Pandapip1 Pandapip1 requested a review from eth-bot as a code owner September 1, 2022 02:29
@Pandapip1 Pandapip1 changed the title Add EIP-5075: Rate Limiter for Token Outflows from Contracts Add EIP-5075: RateLimit, an outflow limiter for assets Sep 1, 2022
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Sep 1, 2022
Pandapip1 and others added 2 commits August 31, 2022 22:29
@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Sep 16, 2022
status: Draft
type: Standards Track
category: ERC
created: 2022-05-05
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
created: 2022-05-05
created: 2022-05-05
requires: 20

Is there a dependency on ERC-20 from this EIP?

@github-actions github-actions bot removed the w-stale Waiting on activity label Sep 25, 2022
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added w-stale Waiting on activity and removed w-stale Waiting on activity labels Oct 9, 2022
@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Oct 24, 2022
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants