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

Validate all Send Addresses against stellar.expert's list of malicious/unsafe addresses #245

Merged
merged 12 commits into from
Jan 11, 2021
Merged
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@
"trezor-connect": "^8.1.16",
"typescript": "~4.0.5"
},
"resolutions": {
"**/@typescript-eslint/eslint-plugin": "^4.1.1",
"**/@typescript-eslint/parser": "^4.1.1"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into a very weird bug with react-scripts. It was falsely throwing an eslint error in a module exporting an array. What I've done here is kind of a band aid fix: it upgrades the versions of typescript-eslint that react-scripts uses. Another option is to update react-scripts itself BUT that causes a whole host of other eslint issues to crop up which were out of scope of this issue. This was the least invasive method I was able to find.

Further reading here:
https://stackoverflow.com/a/63919395
facebook/create-react-app#9515

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen this one before. I think your fix is good. 👍

Copy link

Choose a reason for hiding this comment

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

Yeah Haven't seen resolutions before but that seems like a useful tool for the toolbox!

"scripts": {
"install-if-package-changed": "git diff-tree -r --name-only --no-commit-id ORIG_HEAD HEAD | grep --quiet yarn.lock && yarn install || exit 0",
"start": "react-scripts start",
Expand Down
32 changes: 31 additions & 1 deletion src/components/SendTransaction/CreateTransaction.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { getNetworkConfig } from "helpers/getNetworkConfig";
import { lumensFromStroops } from "helpers/stroopConversion";
import { logEvent } from "helpers/tracking";
import { useRedux } from "hooks/useRedux";
import { useFlaggedAccounts } from "hooks/useUnsafeAccounts";
import {
ActionStatus,
NetworkCongestion,
Expand Down Expand Up @@ -147,6 +148,9 @@ export const CreateTransaction = ({
initialFormData.isAccountFunded,
);

const [isAccountUnsafe, setIsAccountUnsafe] = useState(false);
const [isAccountMalicious, setIsisAccountMalicious] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: there is an extra is in setIsisAccountMalicious.


const knownAccount =
knownAccounts[toAccountId] || knownAccounts[federationAddress || ""];
const [prevAddress, setPrevAddress] = useState(
Expand Down Expand Up @@ -251,6 +255,19 @@ export const CreateTransaction = ({
}
};

const { flaggedAccounts } = useFlaggedAccounts();

const checkIfAccountIsFlagged = (accountId: string) => {
const flaggedAccountData = flaggedAccounts.find(
({ address }: { address: string }) => address === accountId,
);
if (flaggedAccountData?.tags) {
const { tags } = flaggedAccountData;
setIsAccountUnsafe(tags.includes("unsafe"));
setIsisAccountMalicious(tags.includes("malicious"));
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not resetting setIsAccountUnsafe and setIsAccountMalicious to false after it's been set to true, in case the user enters a malicious address followed by a valid one. We could do that at the top of this function.


const checkAndSetIsAccountFunded = async (accountId: string) => {
if (!accountId || !StrKey.isValidEd25519PublicKey(accountId)) {
setIsAccountFunded(true);
Expand Down Expand Up @@ -288,6 +305,9 @@ export const CreateTransaction = ({
) {
message =
'Stellar address or public key is invalid. Public keys are uppercase and begin with letter "G."';
} else if (isAccountMalicious) {
message =
"This account has been flagged as being potentially malicious.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if something is malicious, show an error message and don't allow form submission

}

errors[SendFormIds.SEND_TO] = message;
Expand Down Expand Up @@ -400,7 +420,7 @@ export const CreateTransaction = ({
hasErrors = true;
}
});

debugger;
if (hasErrors) {
setInputErrors(errors);
} else {
Expand Down Expand Up @@ -445,6 +465,8 @@ export const CreateTransaction = ({

setToAccountId(e.target.value);

checkIfAccountIsFlagged(e.target.value);

if (federationAddressFetchStatus) {
setFederationAddressFetchStatus(null);
}
Expand Down Expand Up @@ -515,6 +537,14 @@ export const CreateTransaction = ({
</RowEl>
)}

{isAccountUnsafe && (
piyalbasu marked this conversation as resolved.
Show resolved Hide resolved
<RowEl>
<InfoBlock variant={InfoBlockVariant.error}>
<p>This account has been flagged as being potentially unsafe.</p>
</InfoBlock>
</RowEl>
)}

<RowEl>
<CellEl>
<Input
Expand Down
1 change: 1 addition & 0 deletions src/constants/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import StellarSdk from "stellar-sdk";
export const TX_HISTORY_LIMIT = 100;
export const TX_HISTORY_MIN_AMOUNT = 0.5;
export const RESET_STORE_ACTION_TYPE = "RESET";
export const FLAGGED_ACCOUNT_STORAGE_ID = "flaggedAcounts";

interface NetworkItemConfig {
url: string;
Expand Down
25 changes: 25 additions & 0 deletions src/helpers/getFlaggedAccounts.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
const UNSAFE_ACCOUNTS_URL =
"https://api.stellar.expert/explorer/directory?limit=20000000&tag[]=malicious&tag[]=unsafe";
// setting limit very high as there doesn't appear to be a better way to get all entries from API

const RESPONSE_TIMEOUT = 5000;
// if API doesn't respond in this amount of time, we'll cancel the request

export const getFlaggedAccounts = async () => {
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), RESPONSE_TIMEOUT);

const flaggedAccountsRes = await fetch(UNSAFE_ACCOUNTS_URL, {
signal: controller.signal,
});
clearTimeout(timeoutId);
const flaggedAccountsJson = await flaggedAccountsRes.json();

const {
_embedded: { records: unsafeAccountsData },
} = flaggedAccountsJson;

return unsafeAccountsData.map(
({ address, tags }: { address: string; tags: [] }) => ({ address, tags }),
);
};
39 changes: 39 additions & 0 deletions src/hooks/useUnsafeAccounts.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// @ts-nocheck
import { useEffect, useState } from "react";
import { FLAGGED_ACCOUNT_STORAGE_ID } from "constants/settings";

import { getFlaggedAccounts } from "helpers/getFlaggedAccounts";

interface FlaggedAccount {
address: string;
tags: [string];
}

interface FlaggedAccounts extends Array<FlaggedAccount> {}

export const useFlaggedAccounts = () => {
const [flaggedAccounts, setFlaggedAccounts] = useState<FlaggedAccounts>([
{ address: "", tags: [] },
]);

useEffect(() => {
const fetchFlaggedAccounts = async () => {
let accounts;

try {
accounts = await getFlaggedAccounts();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function will throw an error if it doesn't resolve in time

} catch (e) {
accounts =
JSON.parse(localStorage.getItem(FLAGGED_ACCOUNT_STORAGE_ID)) || [];
}
setFlaggedAccounts(accounts);
localStorage.setItem(
FLAGGED_ACCOUNT_STORAGE_ID,
JSON.stringify(accounts),
);
};
fetchFlaggedAccounts();
}, []);

return { flaggedAccounts };
};
Loading