github-actions[bot]
commented
2 weeks ago Branch preview
✅ Deploy successful!
Storybook: https://address_poisoning--walletweb.review.5afe.dev/storybook/
Closed jmealy closed 1 week ago
github-actions[bot]
commented
2 weeks ago ✅ Deploy successful!
Storybook: https://address_poisoning--walletweb.review.5afe.dev/storybook/
github-actions[bot]
commented
2 weeks ago Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.
| Type | Occurrences | Fixable |
|---|---|---|
| Errors | 0 | 0 |
| Warnings | 0 | 0 |
| Ignored | 0 | N/A |
Report generated by eslint-plus-action
github-actions[bot]
commented
2 weeks ago This analysis was generated by the Next.js Bundle Analysis action. 🤖
| Page | Size (compressed) |
|---|---|
global |
1000.09 KB (🟡 +113 B) |
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!
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 |
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.
github-actions[bot]
commented
2 weeks ago St.:grey_question: |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟡 | Statements | 78.78% (+0.03% 🔼) |
11349/14406 |
| 🔴 | Branches | 58.16% (-0.03% 🔻) |
2745/4720 |
| 🟡 | Functions | 65.75% (+0.04% 🔼) |
1824/2774 |
| 🟢 | Lines | 80.14% (+0.03% 🔼) |
10228/12763 |
1409 tests passing in 195 suites.
Report generated by 🧪jest coverage report action from 944eeeeabc084092989713fe6c0a7e0104ad4d78
github-actions[bot]
commented
1 week ago Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.
| Type | Occurrences | Fixable |
|---|---|---|
| Errors | 0 | 0 |
| Warnings | 0 | 0 |
| Ignored | 0 | N/A |
Report generated by eslint-plus-action
francovenica
commented
1 week ago 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. If it was decided that those were not going to be implemented then it's fine, I'm just asking for clarification
I don't see the "Don't show again" when you copy an address.
I tried sending tokens to a poisoned address to see if a warning popped up in the tx signing, but I didn't see any. I saw a comment in the design that we might not implement it, but I don't see if it was decied or not.
Another question regarding this specific case:
It seems here that some USDC were sent somewhere, and 2 malicious tx were created in the history as well. What I don't get is why one has the red warning label and the other only the "!" icon with a tooltip. what is the difference between these to show the warning differently?
Tx nonce 21:
https://address_poisoning--walletweb.review.5afe.dev/transactions/history?safe=eth:0xf0ddc6435b47d161d46f51f2ea7c5f32f90b98be
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):
We show a warning when the user tries to even copy the address. I think that we should also warn the user if he tries to save the address in the address book, using the "..." buttons.
jmealy
commented
1 week ago I don't see the "Don't show again" when you copy an address.
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
I tried sending tokens to a poisoned address to see if a warning popped up in the tx signing, but I didn't see any. I saw a comment in the design that we might not implement it, but I don't see if it was decied or not.
This is an idea for displaying blockaid warnings, so out of scope for this feature.
Another question regarding this specific case: It seems here that some USDC were sent somewhere, and 2 malicious tx were created in the history as well. What I don't get is why one has the red warning label and the other only the "!" icon with a tooltip. what is the difference between these to show the warning differently? Tx nonce 21: https://address_poisoning--walletweb.review.5afe.dev/transactions/history?safe=eth:0xf0ddc6435b47d161d46f51f2ea7c5f32f90b98be
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?
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): We show a warning when the user tries to even copy the address. I think that we should also warn the user if he tries to save the address in the address book, using the "..." buttons.
I like this idea 👍 And yes, makes sense as a separate ticket I think
github-actions[bot]
commented
1 week ago Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.
| Type | Occurrences | Fixable |
|---|---|---|
| Errors | 0 | 0 |
| Warnings | 0 | 0 |
| Ignored | 0 | N/A |
Report generated by eslint-plus-action
TanyaEfremova
commented
1 week ago francovenica
commented
1 week ago 1 - Noted, no checkbox for the copy address modal 2 - I'll make a ticket after this one is approved 3 - It was noted in THIS thread that the scenario where 2 tx might show different warnings is a limitation on how this was implemented in the cgw side, and it seems it's going to be unavoidable, so nothing to fix here.
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.
For what I see in the code it wasn't implemented. Is this correct?
jmealy
commented
1 week ago 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. For what I see in the code it wasn't implemented. Is this correct?
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?
github-actions[bot]
commented
1 week ago Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.
| Type | Occurrences | Fixable |
|---|---|---|
| Errors | 0 | 0 |
| Warnings | 0 | 0 |
| Ignored | 0 | N/A |
Report generated by eslint-plus-action
francovenica
commented
1 week ago 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. Some of them have the icon of "dubious token" and even copying the address gives you the warning of the tx being an address involved in a tx with a suspicious token, but the warning is not there, so is difficult for me to predict which type of tx should have the red warning message and which one not. I'm aware of a document that describes how this tx are flagged, but that's something the end user will not see or read, so it has to be obvious from the UI itself if a tx is suspicious or not
Take an example the swap in nonce 418 of this safe: https://address_poisoning--walletweb.review.5afe.dev/transactions/history?safe=sep:0x8f4A19C85b39032A37f7a6dCc65234f966F72551
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.
TanyaEfremova
commented
1 week ago 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?
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
Screenshots
Checklist