Blocked people from sending to addresses listed in CityOfZion/phishing #887
Conversation
package.json
Outdated
| @@ -81,6 +81,7 @@ | |||
| "isomorphic-fetch": "2.2.1", | |||
| "lodash": "4.17.4", | |||
| "neon-js": "git+https://github.com/cityofzion/neon-js.git#3.3.3", | |||
| "node-fetch": "^2.1.1", | |||
There was a problem hiding this comment.
We already have axios in this repo -- we should try to avoid bringing in extra packages to prevent bloat.
app/core/wallet.js
Outdated
| let addressBlacklist: Array<string> | null = null | ||
| export const isInBlacklist = (address: string): Promise<boolean> => { | ||
| if (addressBlacklist !== null) return Promise.resolve(addressBlacklist.includes(address)) | ||
| return fetch('https://raw.githubusercontent.com/CityOfZion/phishing/master/blockedAddresses.json') |
There was a problem hiding this comment.
We could write this with axios and async/await syntax in a pretty direct manner:
import axios from 'axios'
export const isInBlacklist = async (address: string): Promise<boolean> => {
if (addressBlacklist === null) {
const { data } = await axios.get('https://raw.githubusercontent.com/CityOfZion/phishing/master/blockedAddresses.json')
addressBlacklist = data
}
return addressBlacklist.includes(address)
}| display: DISPLAY_MODES.CONFIRM | ||
| }) | ||
| } | ||
| isInBlacklist(entry.address).then(inBlacklist => { |
There was a problem hiding this comment.
FYI using async/await syntax here can remove a layer of nesting and improve the readability. We've been favoring this syntax over raw promises throughout the codebase.
You would first need to change the method signature to include async, i.e.:
handleConfirmAddRecipient = async (entry: SendEntryType) => { ... }Then the skeleton of this function simply becomes:
if (await isInBlacklist(entry.address)) {
...
} else {
...
}There was a problem hiding this comment.
Unfortunately, making that function async/await removes access to the "this" variable. I get Cannot read property 'showErrorNotification' of undefined or ReferenceError: _this11 is not defined. There's a weird workaround to this in withAddressCheck.jsx, but applying it breaks other methods within the class. There's another promise ".then" in the same file, presumably for the same reason.
There was a problem hiding this comment.
Ahh, that should be fixable by using async function(...) { ... } instead of async (...) => { ... }. Regardless, I can probably merge this as-is and look into it afterwards.
…f node-fetch/promises
| display: DISPLAY_MODES.CONFIRM | ||
| }) | ||
| } | ||
| isInBlacklist(entry.address).then(inBlacklist => { |
There was a problem hiding this comment.
Ahh, that should be fixable by using async function(...) { ... } instead of async (...) => { ... }. Regardless, I can probably merge this as-is and look into it afterwards.
|
Thought I'd follow up and mention that the reason your function couldn't be changed to use |
What current issue(s) from Trello/Github does this address?
Fixes issue #878
What problem does this PR solve?
Up until this commit, people may have unintentionally sent tokens to malicious addresses
How did you solve this problem?
The first time someone tries to send tokens to an address, neon-wallet fetches the contents of the JSON file at the root of CityOfZion/phishing and compares its contents against the address. If there is a match, the transaction is halted and a message is displayed notifying the user that they just tried to send tokens to a phishing address. Subsequently, the application uses a cached copy of the JSON file.
How did you make sure your solution works?
I tested "AasqbuVKaCARqoWfp64ygM5fxYBqmMMgpq" as my malicious address and "Adr3XjZ5QDzVJrWvzmsTTchpLRRGSzgS5A" as my non-malicious address
Are there any special changes in the code that we should be aware of?
I added node-fetch as a dependency. I was going to add electron-fetch instead, but it was acting up and I didn't want to risk it.