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

Flag "echo" transfers as imitations #2026

Merged
merged 7 commits into from
Oct 23, 2024
Merged

Flag "echo" transfers as imitations #2026

merged 7 commits into from
Oct 23, 2024

Conversation

iamacook
Copy link
Member

Summary

Resolves #2025

An address poisoning vector we don't flag are incoming transfers of the same asset sent of minimal value, or "echoes". When sending a large amount of an asset, it is common to receive a low value of the same asset from a malicious actor in the hopes you copy their address and send to them instead.

This adds new logic to flag such transactions. If the receipt of a token occurs after it being sent, under a defined limit, it is deemed an "echo" and flagges as such.

Changes

  • Add mappings.imitation.echoLimit value
  • Flag "echo" transactions
  • Add appropriate test coverage

@iamacook iamacook self-assigned this Oct 16, 2024
@iamacook iamacook requested a review from a team as a code owner October 16, 2024 13:20
@iamacook iamacook linked an issue Oct 17, 2024 that may be closed by this pull request
hectorgomezv
hectorgomezv previously approved these changes Oct 21, 2024
Copy link
Member

@hectorgomezv hectorgomezv left a comment

Choose a reason for hiding this comment

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

👏🏻👏🏻👏🏻

Comment on lines 110 to 111
const chain = chainBuilder().build();
const safe = safeBuilder().build();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we could move these const to the line 61 (right after const echoLimit = BigInt(10);), so we have all in one place, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved in f17faf5.

Comment on lines +120 to +123
return (
this.isSpoofedEvent(txInfo, prevTxInfo) ||
this.isEchoImitation(txInfo, prevTxInfo)
);
Copy link
Member

Choose a reason for hiding this comment

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

(This is more a general comment, no directly related to the PR code, and I think it's out of the scope)

As this TransferImitationMapper gets more complex, it is likely getting more computationally expensive. I think it's fine for now, but given this mapper is applied to each group of transactions, and if I'm not wrong it's deterministic, it'd be great to somehow cache/memoize whether a given transaction is an imitation one. Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree - we should look into caching the mapped entities (potentially across the project). I've noted it down for discussion.

Base automatically changed from imitation-value-tolerance to main October 23, 2024 10:44
@iamacook iamacook dismissed hectorgomezv’s stale review October 23, 2024 10:44

The base branch was changed.

@iamacook iamacook merged commit c3fc573 into main Oct 23, 2024
20 checks passed
@iamacook iamacook deleted the echo-imitation-flagging branch October 23, 2024 13:06
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.

Flag "echo" incoming transfers
2 participants