-
Notifications
You must be signed in to change notification settings - Fork 471
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
feat: block/warn about bridges accordingly #2652
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
64 changes: 0 additions & 64 deletions
64
src/components/walletconnect/ProposalForm/ChainWarning.tsx
This file was deleted.
Oops, something went wrong.
43 changes: 43 additions & 0 deletions
43
src/components/walletconnect/ProposalForm/CompatibilityWarning.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import { Alert, Typography } from '@mui/material' | ||
import type { ReactElement } from 'react' | ||
import type { Web3WalletTypes } from '@walletconnect/web3wallet' | ||
|
||
import ChainIndicator from '@/components/common/ChainIndicator' | ||
import { useCompatibilityWarning } from './useCompatibilityWarning' | ||
import useSafeInfo from '@/hooks/useSafeInfo' | ||
|
||
import css from './styles.module.css' | ||
|
||
export const CompatibilityWarning = ({ | ||
proposal, | ||
chainIds, | ||
}: { | ||
proposal: Web3WalletTypes.SessionProposal | ||
chainIds: Array<string> | ||
}): ReactElement => { | ||
const { safe } = useSafeInfo() | ||
const isUnsupportedChain = !chainIds.includes(safe.chainId) | ||
const { severity, message } = useCompatibilityWarning(proposal, isUnsupportedChain) | ||
|
||
return ( | ||
<> | ||
<Alert severity={severity} className={css.alert}> | ||
{message} | ||
</Alert> | ||
|
||
{isUnsupportedChain && ( | ||
<> | ||
<Typography mt={3} mb={1}> | ||
Supported networks | ||
</Typography> | ||
|
||
<div> | ||
{chainIds.map((chainId) => ( | ||
<ChainIndicator inline chainId={chainId} key={chainId} className={css.chain} /> | ||
))} | ||
</div> | ||
</> | ||
)} | ||
</> | ||
) | ||
} |
118 changes: 118 additions & 0 deletions
118
src/components/walletconnect/ProposalForm/__tests__/useCompatibilityWarning.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
import { renderHook } from '@/tests/test-utils' | ||
import type { ChainInfo, SafeInfo } from '@safe-global/safe-gateway-typescript-sdk' | ||
import type { Web3WalletTypes } from '@walletconnect/web3wallet' | ||
|
||
import * as bridges from '../bridges' | ||
import { useCompatibilityWarning } from '../useCompatibilityWarning' | ||
|
||
describe('useCompatibilityWarning', () => { | ||
it('should return an error for a dangerous bridge', () => { | ||
jest.spyOn(bridges, 'isDangerousBridge').mockReturnValue(true) | ||
|
||
const proposal = { | ||
params: { proposer: { metadata: { name: 'Fake Bridge' } } }, | ||
verifyContext: { verified: { origin: '' } }, | ||
} as unknown as Web3WalletTypes.SessionProposal | ||
|
||
const { result } = renderHook(() => useCompatibilityWarning(proposal, false)) | ||
|
||
expect(result.current).toEqual({ | ||
message: | ||
'Fake Bridge is a bridge that is unusable in Safe{Wallet} due to the current implementation of WalletConnect — the bridged funds will be lost. Consider using a different bridge.', | ||
severity: 'error', | ||
}) | ||
}) | ||
|
||
it('should return a warning for a risky bridge', () => { | ||
jest.spyOn(bridges, 'isDangerousBridge').mockReturnValue(false) | ||
jest.spyOn(bridges, 'isRiskyBridge').mockReturnValue(true) | ||
|
||
const proposal = { | ||
params: { proposer: { metadata: { name: 'Fake Bridge' } } }, | ||
verifyContext: { verified: { origin: '' } }, | ||
} as unknown as Web3WalletTypes.SessionProposal | ||
|
||
const { result } = renderHook(() => useCompatibilityWarning(proposal, false)) | ||
|
||
expect(result.current).toEqual({ | ||
message: | ||
'While using Fake Bridge, please make sure that the desination address you send funds to matches the Safe address you have on the respective chain. Otherwise, the funds will be lost.', | ||
severity: 'warning', | ||
}) | ||
}) | ||
|
||
it('should return an error for an unsupported chain', () => { | ||
jest.spyOn(bridges, 'isDangerousBridge').mockReturnValue(false) | ||
jest.spyOn(bridges, 'isRiskyBridge').mockReturnValue(false) | ||
|
||
const proposal = { | ||
params: { proposer: { metadata: { name: 'Fake dApp' } } }, | ||
verifyContext: { verified: { origin: '' } }, | ||
} as unknown as Web3WalletTypes.SessionProposal | ||
|
||
const { result } = renderHook(() => useCompatibilityWarning(proposal, true)) | ||
|
||
expect(result.current).toEqual({ | ||
message: | ||
'Fake dApp does not support the Safe Account network. If you want to interact with Fake dApp, please switch to a Safe Account on a supported network.', | ||
severity: 'error', | ||
}) | ||
}) | ||
|
||
describe('should otherwise return info', () => { | ||
it('if chains are loaded', () => { | ||
jest.spyOn(bridges, 'isDangerousBridge').mockReturnValue(false) | ||
jest.spyOn(bridges, 'isRiskyBridge').mockReturnValue(false) | ||
|
||
const proposal = { | ||
params: { proposer: { metadata: { name: 'Fake dApp' } } }, | ||
verifyContext: { verified: { origin: '' } }, | ||
} as unknown as Web3WalletTypes.SessionProposal | ||
|
||
const { result } = renderHook(() => useCompatibilityWarning(proposal, false), { | ||
initialReduxState: { | ||
chains: { | ||
loading: false, | ||
error: undefined, | ||
data: [ | ||
{ | ||
chainId: '1', | ||
chainName: 'Ethereum', | ||
}, | ||
] as unknown as Array<ChainInfo>, | ||
}, | ||
safeInfo: { | ||
loading: false, | ||
error: undefined, | ||
data: { | ||
address: {}, | ||
chainId: '1', | ||
} as unknown as SafeInfo, | ||
}, | ||
}, | ||
}) | ||
|
||
expect(result.current).toEqual({ | ||
message: 'Please make sure that the dApp is connected to Ethereum.', | ||
severity: 'info', | ||
}) | ||
}) | ||
|
||
it("if chains aren't loaded", () => { | ||
jest.spyOn(bridges, 'isDangerousBridge').mockReturnValue(false) | ||
jest.spyOn(bridges, 'isRiskyBridge').mockReturnValue(false) | ||
|
||
const proposal = { | ||
params: { proposer: { metadata: { name: 'Fake dApp' } } }, | ||
verifyContext: { verified: { origin: '' } }, | ||
} as unknown as Web3WalletTypes.SessionProposal | ||
|
||
const { result } = renderHook(() => useCompatibilityWarning(proposal, false)) | ||
|
||
expect(result.current).toEqual({ | ||
message: 'Please make sure that the dApp is connected to this network.', | ||
severity: 'info', | ||
}) | ||
}) | ||
}) | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
const DangerousBridges = [ | ||
'bridge.arbitrum.io', | ||
'bridge.base.org', | ||
'cbridge.celer.network', | ||
'www.orbiter.finance', | ||
'zksync-era.l2scan.co', | ||
'app.optimism.io', | ||
'www.portalbridge.com', | ||
'wallet.polygon.technology', | ||
'app.rhino.fi', | ||
] | ||
|
||
const RiskyBridges = [ | ||
'across.to', | ||
'app.allbridge.io', | ||
'core.allbridge.io', | ||
'bungee.exchange', | ||
'www.carrier.so', | ||
'app.chainport.io', | ||
'bridge.gnosischain.com', | ||
'app.hop.exchange', | ||
'app.interport.fi', | ||
'jumper.exchange', | ||
'www.layerswap.io', | ||
'meson.fi', | ||
'satellite.money', | ||
'stargate.finance', | ||
'app.squidrouter.com', | ||
'app.symbiosis.finance', | ||
'www.synapseprotocol.com', | ||
'app.thevoyager.io', | ||
'portal.txsync.io', | ||
'bridge.wanchain.org', | ||
'app.xy.finance', | ||
] | ||
|
||
export const isDangerousBridge = (origin: string) => { | ||
return DangerousBridges.some((bridge) => origin.includes(bridge)) | ||
} | ||
|
||
export const isRiskyBridge = (origin: string) => { | ||
return RiskyBridges.some((bridge) => origin.includes(bridge)) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
64 changes: 64 additions & 0 deletions
64
src/components/walletconnect/ProposalForm/useCompatibilityWarning.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import { useMemo } from 'react' | ||
import type { AlertColor } from '@mui/material' | ||
import type { Web3WalletTypes } from '@walletconnect/web3wallet' | ||
|
||
import useChains from '@/hooks/useChains' | ||
import useSafeInfo from '@/hooks/useSafeInfo' | ||
import { isDangerousBridge, isRiskyBridge } from './bridges' | ||
|
||
const NAME_PLACEHOLDER = '%%name%%' | ||
const CHAIN_PLACEHOLDER = '%%chain%%' | ||
|
||
const Warnings: Record<string, { severity: AlertColor; message: string }> = { | ||
DANGEROUS_BRIDGE: { | ||
severity: 'error', | ||
message: `${NAME_PLACEHOLDER} is a bridge that is unusable in Safe{Wallet} due to the current implementation of WalletConnect — the bridged funds will be lost. Consider using a different bridge.`, | ||
}, | ||
RISKY_BRIDGE: { | ||
severity: 'warning', | ||
message: `While using ${NAME_PLACEHOLDER}, please make sure that the desination address you send funds to matches the Safe address you have on the respective chain. Otherwise, the funds will be lost.`, | ||
}, | ||
UNSUPPORTED_CHAIN: { | ||
severity: 'error', | ||
message: `${NAME_PLACEHOLDER} does not support the Safe Account network. If you want to interact with ${NAME_PLACEHOLDER}, please switch to a Safe Account on a supported network.`, | ||
}, | ||
WRONG_CHAIN: { | ||
severity: 'info', | ||
message: `Please make sure that the dApp is connected to ${CHAIN_PLACEHOLDER}.`, | ||
}, | ||
} | ||
|
||
export const useCompatibilityWarning = ( | ||
proposal: Web3WalletTypes.SessionProposal, | ||
isUnsupportedChain: boolean, | ||
): (typeof Warnings)[string] => { | ||
const { configs } = useChains() | ||
const { safe } = useSafeInfo() | ||
|
||
return useMemo(() => { | ||
const { origin } = proposal.verifyContext.verified | ||
const { proposer } = proposal.params | ||
|
||
let { message, severity } = isDangerousBridge(origin) | ||
? Warnings.DANGEROUS_BRIDGE | ||
: isRiskyBridge(origin) | ||
? Warnings.RISKY_BRIDGE | ||
: isUnsupportedChain | ||
? Warnings.UNSUPPORTED_CHAIN | ||
: Warnings.WRONG_CHAIN | ||
|
||
if (message.includes(NAME_PLACEHOLDER)) { | ||
message = message.replaceAll(NAME_PLACEHOLDER, proposer.metadata.name) | ||
} | ||
|
||
if (message.includes(CHAIN_PLACEHOLDER)) { | ||
const chainName = configs.find((chain) => chain.chainId === safe.chainId)?.chainName ?? 'this network' | ||
message = message.replaceAll(CHAIN_PLACEHOLDER, chainName) | ||
} | ||
|
||
return { | ||
message, | ||
severity, | ||
} | ||
}, [configs, isUnsupportedChain, proposal.params, proposal.verifyContext.verified, safe.chainId]) | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming-wise, there's nothing dangerous/risky about these bridges per se, it's just that they aren't smart contract account-friendly. Please find more specific, non-sensationalistic, names for the two arrays. And please move these lists (not the two functions below) to
src/components/walletconnect/constants.ts
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 398638f.