-
Notifications
You must be signed in to change notification settings - Fork 73
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
Flag spoof events with varied value #2024
Conversation
* @param {Erc20TransferTransactionInfo} prevTxInfo - previous transaction info | ||
* @returns {boolean} - whether the transaction is an imitation | ||
*/ | ||
isSpoofedEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this to it's own method in preparation for a forthcoming PR that detects another attack vector.
} | ||
const chain = chainBuilder().build(); | ||
const safe = safeBuilder().build(); | ||
describe('Event spoofing', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest hiding whitespace to review this. The test content has not drastically changed for the existing tests, but newer value-tolerance-focused tests have been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏🏻👏🏻👏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with some suggestions.
src/routes/transactions/transactions-history.imitation-transactions.controller.spec.ts
Show resolved
Hide resolved
src/routes/transactions/transactions-history.imitation-transactions.controller.spec.ts
Show resolved
Hide resolved
src/routes/transactions/transactions-history.imitation-transactions.controller.spec.ts
Show resolved
Hide resolved
Question: Can we calculate the tolerance dynamically from the recent transactions of the sender? |
We only have the current page for comparison so it's not a sufficient sample size imo. If the page only had high value transactions, for example, there'd be a higher chance of false positives/ |
@iamacook Thanks for the explanation, I'll look into it more to see if there is anything else we can do to make it even stronger. |
Summary
Resolves #2023
We flag imitation transactions (spoof events) based on certain criteria, of which a matching value being one of them. This comparison is becoming redundant as the values are beginning to vary compared to the transaction being imitated.
This adjusts the current check, adding a new tolerance. A transaction is now marked as an imitation if the value is +/- value + tolerance of the transaction being imitated.
Changes