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

Rate Limit Bridge #836

Open
0xmovses opened this issue Nov 11, 2024 · 9 comments · May be fixed by #851
Open

Rate Limit Bridge #836

0xmovses opened this issue Nov 11, 2024 · 9 comments · May be fixed by #851
Assignees
Labels

Comments

@0xmovses
Copy link
Contributor

0xmovses commented Nov 11, 2024

Summary

We need to limit the bridge's operations for initial deployment, this is to secure regular flow, safety and maintainability. It is also to ensure we can always recover from an internal exploit (relayer liveness no longer a threat due to refund call restrictions).

We want to limit the amount of transfers possible within a 24h period by volume. This volume amount must be equal to the max amount we can mint on demand.

  • Amount must be set on config
  • Logic implemented at the relayer level (this to avoid additional changes to contracts)
  • Every 24 hour period that passes, relayer increments on volume, if rate limit has been exceeded before current 24 hour period has passed, lock calls are blocked on the relayer and an appropriate Error is bubbled up.

NB. Implementing this at the relayer level is for initial deployment, after this we want to implement this at the contract level via an upgrade with a governing body would be able to vote on proposals to rate limit, that is the volume and the time period.

@0xmovses 0xmovses changed the title Rate Limiting Rate Limit Bridge Nov 11, 2024
@musitdev
Copy link
Contributor

musitdev commented Nov 12, 2024

Blocking Lock event shouldn't be done this way:

  • It blocks the transfer: The user has sent an initiate_transfer Tx, but he doesn't know the lock has been blocked. He won't be able to get the transfer and will wait without knowing why until the lock releases the fund. He cannot get his money back soon.
  • It's unclear what is done with the lock event: it's disregarded, put aside to wait for the end of the 24h, and executed after? This part should be detailed.
  • a security issue has been found if the lock_transfer is not called in time.

@0xmovses
Copy link
Contributor Author

0xmovses commented Nov 12, 2024

a security issue has been found if the lock_transfer is not called in time.

"In time" meaning before the timelock has expired, or something else?

I was thinking that the front-end could simple be notified. that the contract has been locked.

We could instead, do a lock for initiateBridgeTransfer but the only way to do this would mean updating the contract itself and having the rate limit set in the contract, which I don't think is a bad thing. What do you think?

@musitdev
Copy link
Contributor

I think the contract should reject the initiate_tranfer when the limit is reached. There are different possibilities to indicate to the contract to reject initial_transfer. The contract can do the calculus but it needs to be very strict in time or we indicate a number of blocks. The other way is the relayer detects that the limit has been reached and calls the contract to put it in a locked mode.

@musitdev
Copy link
Contributor

The scenario can be:

  • Users call Initiate transfer
  • The relayer gets Initiate_transfer event and increases the current amount transferred with the new transfer event.
  • The relayer detects that the limit is reached.
  • The relayer calls the contract to put it in lock mode.
  • The relayer still processes all events as before.
  • The relayer detects the rate limit time is finished and calls the contract to remove the lock mode.
  • The relayer sets the current amount transferred to zero.

@0xmovses
Copy link
Contributor Author

So some onlyOwner lock function that the bridge operator can call. Sounds good, I'll implement like this. Thanks for your thoughts!

@bhenhsi bhenhsi linked a pull request Nov 13, 2024 that will close this issue
@Primata
Copy link
Contributor

Primata commented Nov 20, 2024

Have two options to present here:

  1. it stores how much budget has been used daily by using a DateTimeLibrary
  2. if the difference between the last reset and current time is higher than the window we choose (here it's 1 days but it could be a variable set by us), it resets the time and the budget on both ways.

Second option is a little bit cheaper in average (it will be more expensive for users that bridge during a reset) but it's on average cheaper.

function _rateLimitIncoming(uint256 amount) internal {
        (uint256 year, uint256 month, uint256 day) = block.timestamp.timestampToDate();
        incomingRateLimitBudget[year][month][day] += amount;
        require(incomingRateLimitBudget[year][month][day] <= incomingRateLimit, IncomingRateLimitExceeded());
    }
function _rateLimitIncoming(uint256 amount) internal {
        _resetTime();
        incomingRateLimitBudget += amount;
        require(incomingRateLimitBudget <= outboundRateLimit, IncomingRateLimitExceeded());
    }

    function _resetTime() internal {
        if (block.timestamp - lastReset >= 1 days) {
            lastReset = block.timestamp;
            outboundRateLimitBudget = 0;
            incomingRateLimitBudget = 0;
        }
    }

reset after time passed

| src/NativeBridge.sol:NativeBridge contract |                 |         |        |         |         |
|--------------------------------------------|-----------------|---------|--------|---------|---------|
| Deployment Cost                            | Deployment Size |         |        |         |         |
| 1186577                                    | 5332            |         |        |         |         |
| Function Name                              | min             | avg     | median | max     | # calls |
| batchCompleteBridgeTransfer                | 17741           | 1350969 | 76588  | 5273683 | 512     |
| completeBridgeTransfer                     | 3123            | 37002   | 32256  | 89867   | 1536    |
| idsToIncomingNonces                        | 524             | 524     | 524    | 524     | 13087   |
| initialize                                 | 168107          | 168107  | 168107 | 168107  | 3       |
| initiateBridgeTransfer                     | 2531            | 74476   | 37063  | 183835  | 768     |
| noncesToOutgoingTransfers                  | 944             | 944     | 944    | 944     | 256     |

daily record

| src/NativeBridge.sol:NativeBridge contract |                 |         |        |         |         |
|--------------------------------------------|-----------------|---------|--------|---------|---------|
| Deployment Cost                            | Deployment Size |         |        |         |         |
| 1359377                                    | 6131            |         |        |         |         |
| Function Name                              | min             | avg     | median | max     | # calls |
| batchCompleteBridgeTransfer                | 19393           | 1483221 | 78240  | 5647872 | 512     |
| completeBridgeTransfer                     | 3123            | 38471   | 34019  | 91630   | 1536    |
| idsToIncomingNonces                        | 546             | 546     | 546    | 546     | 13433   |
| initialize                                 | 168107          | 168107  | 168107 | 168107  | 3       |
| initiateBridgeTransfer                     | 2531            | 75651   | 38826  | 185598  | 768     |
| noncesToOutgoingTransfers                  | 944             | 944     | 944    | 944     | 256     |

Wondering what would be better or if there are any logical issues with any of those. Implementations under https://github.com/movementlabsxyz/movement/tree/primata/rate-limiter

@0xmovses
Copy link
Contributor Author

Option 1 is better, in my opinion; it's clearer to track the time durations (useful for auditing or analysing historical data) and its more fair to all users. Option 1 is more expensive yes, but the additional cost seems justified for the added precision and fairness. And, its marginally higher.

@andygolay
Copy link
Contributor

Between the above options, I like option 1. However, there appears to still be a risk of large amounts of downtime for the bridge, if a few big transfers go through and use up the daily budget quickly.

Is there a plan already for rate-limiting more granularly individual wallets, IP addresses, and/ or starting off with some limit to the amount per transfer?

@0xmovses
Copy link
Contributor Author

0xmovses commented Nov 20, 2024

I wouldn't consider this a risk, but the rate-limiter simple doing its job. For safety's sake we want to limit the bridge by volume for a given time period. If it gets used up quickly, then its functioning as intended and it is safely controlling flow of volume and traffic.

We just need to set a large enough volume so that we allow the level of activity we expect within 24 hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants