Skip to content

Conversation

@to-sh-1
Copy link
Contributor

@to-sh-1 to-sh-1 commented Jul 27, 2022

Copy link
Collaborator

@0xean 0xean left a comment

Choose a reason for hiding this comment

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

I think this attack is super trivial. In the current implementation if the amount = 1, the amountMinusFee will be 0 which, afaict has no ill effects really.

Just trying to confirm there are no edge cases that can break this, otherwise I think the fix LGTM

@to-sh-1
Copy link
Contributor Author

to-sh-1 commented Aug 5, 2022

I think this attack is super trivial. In the current implementation if the amount = 1, the amountMinusFee will be 0 which, afaict has no ill effects really.

Just trying to confirm there are no edge cases that can break this, otherwise I think the fix LGTM

I agree, I just think rounding the fee up vs down does make sense in case this does get exported to a L2 with 0 fees. We could hold off merging if we wanted as this will have no benefit on the ETH blockchain

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.

3 participants