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

Update restrictions #648

Merged
merged 2 commits into from
May 10, 2022
Merged

Update restrictions #648

merged 2 commits into from
May 10, 2022

Conversation

ChaitanyaKonda
Copy link
Contributor

Description:
If a user does two deposits with the max deposit amount, then a transfer, the user already owns a commitment of value bigger than the max withdraw limit. To control this, this PR introduces max deposit amount to be a quarter of max withdraw amount.

Other fixes:

  • It also fixes restriction tests which always pass despite depositing/withdrawing a value over or under the restriction limit because
    • For a successful test, the test expects to catch an error message Transaction has been reverted by the EVM. This error message is also thrown by the test by calling expect.fail('Transaction has not been reverted by the EVM'); Which meant the test would work even if deposit/withdraw calls were commented out because the error was always thrown and caught as expected.
    • Also, the values used for deposit or withdraw tests were always below the max limit. So the tests created valid deposit and withdraw transactions which did not revert on the blockchain as they were under the max deposit and withdraw restrictions
  • Updated the contracts such that if 1000 MATIC is set as the withdraw limit, 1000 MATIC can be withdrawn successfully whilst 1001 will fail. Earlier only values <1000 MATIC were accepted and values equal to and over 1000 MATIC reverted.

Test:
Automatically done by Github Action.
For a further review, look in blockchain logs of ganache test for transactions with revert messages
Value is above current restrictions for deposits and Value is above current restrictions for withdrawals

Copy link
Contributor

@druiz0992 druiz0992 left a comment

Choose a reason for hiding this comment

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

Why are we limiting withdraws at all? Is it some type of safeguard?

@ChaitanyaKonda
Copy link
Contributor Author

Why are we limiting withdraws at all? Is it some type of safeguard?

It is. In the event of an attack, this will control how much can be withdrawn at once.

@Westlad Westlad added the One more approval needed One reviewer has approved this PR but another is needed label May 10, 2022
@druiz0992 druiz0992 merged commit 2e9534e into master May 10, 2022
@druiz0992 druiz0992 deleted the chait/restrictions2 branch May 10, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
One more approval needed One reviewer has approved this PR but another is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants