Feat: Show warning for address poisoning transactions #3851
Conversation
Branch preview✅ Deploy successful! Storybook: |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
1000.09 KB (🟡 +113 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
Six Pages Changed Size
The following pages changed size from the code in this PR compared to its base branch:
| Page | Size (compressed) | First Load |
|---|---|---|
/apps/open |
53.39 KB (🟡 +31 B) |
1.03 MB |
/transactions |
73.39 KB (🟢 -367 B) |
1.05 MB |
/transactions/history |
73.35 KB (🟢 -368 B) |
1.05 MB |
/transactions/messages |
37.87 KB (🟡 +31 B) |
1.01 MB |
/transactions/queue |
30.98 KB (🟡 +31 B) |
1.01 MB |
/transactions/tx |
20.74 KB (🟡 +31 B) |
1020.83 KB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1409 tests passing in 195 suites. Report generated by 🧪jest coverage report action from 944eeee |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
I got some question regarding things that are in the designs and are not here, and I don't see a discussion where it was decided not to be implemented.
Another question regarding this specific case: A suggestion (see if it can be implemented or we can discuss it in another ticket if there are doubts on if it should be implemented or not): |
For the don't show again checkbox, we discussed it in standup this morning and decided it would be better to be safe and show the warning every time. Unless we get complaints about it being annoying then we can leave it out. @TanyaEfremova
This is an idea for displaying blockaid warnings, so out of scope for this feature.
These two transactions do look the same so I don't know why one was flagged and the other not. @iamacook do you know what the difference is here?
I like this idea 👍 And yes, makes sense as a separate ticket I think |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
|
1 - Noted, no checkbox for the copy address modal question: I tried also to see if this warning message would popup in a transaction when dealing with suspicious addresses (the ones that you get a warning if you try to copy them) but I was never able to make it show up. |
That's right, the scope for this PR is just the warning on transactions and not for addresses specifically. Using the current detection on the gateway for this you'd need to search through the entire tx history to see if an address was involved in an address poisoning attack, which isn't practical. I don't know what the plan is for this warning when sending tokens exactly but my understanding is that the detection would be coming from blockaid, not from our own detection on the gateway. @TanyaEfremova could you give more context on this maybe? |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Leaving this comment after merging I think the implementation on our side is fine, I see the warning just fine in the tx but I'm still effy about how the cgw detects these tx. Take an example the swap in nonce 418 of this safe: Here you have the icon of dubious token, and if you try to copy the address you get the warning, but there is no warning message. |
|
The warning should come from our side, Blockaid would detect it differently. We should still warn users in one way or another about a malicious address they may be interacting with. Only transactions that interact with the poisoning address should have that warning. If we detect them in the tx list, can we also detect the same way in the transaction summary? |
| @@ -51,6 +54,7 @@ const TransferTxInfo = ({ txInfo, txStatus, trusted }: TransferTxInfoProps & { t | |||
| <TransferActions address={address.value} txInfo={txInfo} trusted={trusted} /> | |||
| </EthHashInfo> | |||
There was a problem hiding this comment.
@jmealy
I know this is already closed but the trusted flag is used for address poisoning as well here.
I think we should pass the new imitation flag also into the EthHashInfo so we get the "Before you copy" modal if someone tries to copy the address.
<EthHashInfo
address={address.value}
name={address.name}
customAvatar={address.logoUri}
shortAddress={false}
hasExplorer
showCopyButton
trusted={trusted && !imitation}
>
What it solves
Resolves #3832
How this PR fixes it
Check for transactions that have been flagged as imitations of previous legitimate transactions. If they are flagged, display a warning on the tx in the history.
How to test it
https://www.figma.com/design/ptTs6lDBeUuLNySroJ5PiF/Web-Master-File?node-id=11151-49354&t=gfp1I4vZwEzFOo7i-1
Screenshots
Checklist