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

feat: restrict safe pass safe app access for ofac blocked addresses #4066

Merged
merged 8 commits into from
Oct 14, 2024
14 changes: 11 additions & 3 deletions src/components/common/BlockedAddress/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@ import { useRouter } from 'next/router'
import Disclaimer from '@/components/common/Disclaimer'
import { AppRoutes } from '@/config/routes'

export const BlockedAddress = ({ address }: { address?: string }): ReactElement => {
export const BlockedAddress = ({
address,
featureName,
onClose,
}: {
address?: string
featureName: string
onClose?: () => void
}): ReactElement => {
const theme = useTheme()
const isMobile = useMediaQuery(theme.breakpoints.down('sm'))
const displayAddress = address && isMobile ? shortenAddress(address) : address
Expand All @@ -19,8 +27,8 @@ export const BlockedAddress = ({ address }: { address?: string }): ReactElement
<Disclaimer
title="Blocked address"
subtitle={displayAddress}
content="The above address is part of the OFAC SDN list and the embedded swaps feature with CoW Swap is unavailable for sanctioned addresses."
onAccept={handleAccept}
content={`The above address is part of the OFAC SDN list and the ${featureName} is unavailable for sanctioned addresses.`}
onAccept={onClose ?? handleAccept}
/>
)
}
Expand Down
19 changes: 18 additions & 1 deletion src/components/safe-apps/AppFrame/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { type AddressBookItem, Methods } from '@safe-global/safe-apps-sdk'
import type { ReactElement } from 'react'
import { useMemo } from 'react'
import { useCallback, useEffect } from 'react'
import { CircularProgress, Typography } from '@mui/material'
import { Box, CircularProgress, Typography } from '@mui/material'
import { useRouter } from 'next/router'
import Head from 'next/head'
import type { RequestId } from '@safe-global/safe-apps-sdk'
Expand All @@ -28,6 +28,8 @@ import { PermissionStatus, type SafeAppDataWithPermissions } from '@/components/
import css from './styles.module.css'
import SafeAppIframe from './SafeAppIframe'
import { useCustomAppCommunicator } from '@/hooks/safe-apps/useCustomAppCommunicator'
import { useSanctionedAddress } from '@/hooks/useSanctionedAddress'
import BlockedAddress from '@/components/common/BlockedAddress'

const UNKNOWN_APP_NAME = 'Unknown Safe App'

Expand All @@ -43,6 +45,8 @@ const AppFrame = ({ appUrl, allowedFeaturesList, safeAppFromManifest }: AppFrame
const chainId = useChainId()
const chain = useCurrentChain()
const router = useRouter()
const sanctionedAddress = useSanctionedAddress()
katspaugh marked this conversation as resolved.
Show resolved Hide resolved
const isSafePassApp = appUrl.startsWith('https://community.safe.global')
katspaugh marked this conversation as resolved.
Show resolved Hide resolved
const {
expanded: queueBarExpanded,
dismissedByUser: queueBarDismissed,
Expand Down Expand Up @@ -116,6 +120,19 @@ const AppFrame = ({ appUrl, allowedFeaturesList, safeAppFromManifest }: AppFrame
return <div />
}

if (Boolean(sanctionedAddress) && isSafePassApp) {
return (
<>
<Head>
<title>{`Safe Apps - Viewer - ${remoteApp ? remoteApp.name : UNKNOWN_APP_NAME}`}</title>
</Head>
<Box p={2}>
<BlockedAddress address={sanctionedAddress} featureName="Safe{Pass} Safe app" />
</Box>
</>
)
}

return (
<>
<Head>
Expand Down
2 changes: 1 addition & 1 deletion src/features/swap/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ const SwapWidget = ({ sell }: Params) => {
useCustomAppCommunicator(iframeRef, appData, chain)

if (blockedAddress) {
return <BlockedAddress address={blockedAddress} />
return <BlockedAddress address={blockedAddress} featureName="embedded swaps feature with CoW Swap" />
}

if (!isConsentAccepted) {
Expand Down
18 changes: 17 additions & 1 deletion src/features/walletconnect/components/WcProposalForm/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getPeerName,
getSupportedChainIds,
isBlockedBridge,
isSafePassApp,
isWarnedBridge,
} from '@/features/walletconnect/services/utils'
import { WalletConnectContext } from '@/features/walletconnect/WalletConnectContext'
Expand All @@ -20,6 +21,8 @@ import { type Dispatch, type SetStateAction, useCallback, useContext, useEffect,
import { CompatibilityWarning } from './CompatibilityWarning'
import ProposalVerification from './ProposalVerification'
import css from './styles.module.css'
import { useSanctionedAddress } from '@/hooks/useSanctionedAddress'
import BlockedAddress from '@/components/common/BlockedAddress'

type ProposalFormProps = {
proposal: Web3WalletTypes.SessionProposal
Expand All @@ -38,13 +41,22 @@ const WcProposalForm = ({ proposal, setProposal, onApprove }: ProposalFormProps)
const { isScam, origin } = proposal.verifyContext.verified
const url = proposer.metadata.url || origin

const sanctionedAddress = useSanctionedAddress()
katspaugh marked this conversation as resolved.
Show resolved Hide resolved
const isSafePass = isSafePassApp(origin)

const chainIds = useMemo(() => getSupportedChainIds(configs, proposal.params), [configs, proposal.params])
const isUnsupportedChain = !chainIds.includes(chainId)

const name = getPeerName(proposer) || 'Unknown dApp'
const isHighRisk = proposal.verifyContext.verified.validation === 'INVALID' || isWarnedBridge(origin, name)
const isBlocked = isScam || isBlockedBridge(origin)
const disabled = !safeLoaded || isUnsupportedChain || isBlocked || (isHighRisk && !understandsRisk) || !!isLoading
const disabled =
!safeLoaded ||
isUnsupportedChain ||
isBlocked ||
(isHighRisk && !understandsRisk) ||
!!isLoading ||
(Boolean(sanctionedAddress) && isSafePass)

// On session reject
const onReject = useCallback(async () => {
Expand Down Expand Up @@ -134,6 +146,10 @@ const WcProposalForm = ({ proposal, setProposal, onApprove }: ProposalFormProps)
/>
)}

{isSafePass && Boolean(sanctionedAddress) && (
<BlockedAddress address={sanctionedAddress} featureName="Safe{Pass}" onClose={onReject} />
)}

<Divider flexItem className={css.divider} />

<div className={css.buttons}>
Expand Down
11 changes: 8 additions & 3 deletions src/features/walletconnect/components/WcSessionManager/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import WcErrorMessage from '../WcErrorMessage'
import WcProposalForm from '../WcProposalForm'
import { trackEvent } from '@/services/analytics'
import { WALLETCONNECT_EVENTS } from '@/services/analytics/events/walletconnect'
import { splitError } from '@/features/walletconnect/services/utils'
import { isSafePassApp, splitError } from '@/features/walletconnect/services/utils'
import { useSanctionedAddress } from '@/hooks/useSanctionedAddress'

type WcSessionManagerProps = {
sessions: SessionTypes.Struct[]
Expand All @@ -29,6 +30,7 @@ const WcSessionManager = ({ sessions, uri }: WcSessionManagerProps) => {
const { safe, safeAddress } = useSafeInfo()
const { chainId } = safe
const [proposal, setProposal] = useState<Web3WalletTypes.SessionProposal>()
const sanctionedAddress = useSanctionedAddress()

// On session approve
const onApprove = useCallback(
Expand Down Expand Up @@ -91,15 +93,18 @@ const WcSessionManager = ({ sessions, uri }: WcSessionManagerProps) => {
return walletConnect.onSessionPropose((proposalData) => {
setError(null)

if (autoApprove[chainId]?.[proposalData.verifyContext.verified.origin]) {
if (
autoApprove[chainId]?.[proposalData.verifyContext.verified.origin] &&
(!isSafePassApp(proposalData.verifyContext.verified.origin) || !sanctionedAddress)
) {
onApprove(proposalData)
return
}

setProposal(proposalData)
setIsLoading(undefined)
})
}, [autoApprove, chainId, onApprove, setError, setIsLoading, walletConnect])
}, [autoApprove, chainId, onApprove, setError, setIsLoading, walletConnect, sanctionedAddress])

// Track errors
useEffect(() => {
Expand Down
4 changes: 4 additions & 0 deletions src/features/walletconnect/services/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ export const isBlockedBridge = (origin: string) => {
return BlockedBridges.some((bridge) => origin.includes(bridge))
}

export const isSafePassApp = (origin: string) => {
return origin.includes('community.safe.global')
Copy link
Member

Choose a reason for hiding this comment

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

Can we if not avoid hardcode then at least make it a constant?

Copy link
Member

Choose a reason for hiding this comment

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

Can we use the constant SAFE_PASS_URL here?

}

// Bridge defaults to same address on destination chain but allows changing it
export const isWarnedBridge = (origin: string, name: string) => {
return WarnedBridges.some((bridge) => origin.includes(bridge)) || WarnedBridgeNames.includes(name)
Expand Down
70 changes: 70 additions & 0 deletions src/hooks/__tests__/useSanctionedAddress.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { renderHook } from '@/tests/test-utils'
import { useSanctionedAddress } from '../useSanctionedAddress'
import useSafeAddress from '../useSafeAddress'
import useWallet from '../wallets/useWallet'
import { faker } from '@faker-js/faker'
import { connectedWalletBuilder } from '@/tests/builders/wallet'
import * as ofac from '@/store/ofac'

jest.mock('@/hooks/useSafeAddress')
jest.mock('@/hooks/wallets/useWallet')

describe('useSanctionedAddress', () => {
const mockUseSafeAddress = useSafeAddress as jest.MockedFunction<typeof useSafeAddress>
const mockUseWallet = useWallet as jest.MockedFunction<typeof useWallet>

it('should return undefined without safeAddress and wallet', () => {
const { result } = renderHook(() => useSanctionedAddress())
expect(result.current).toBeUndefined()
})

it('should return undefined if neither safeAddress nor wallet are sanctioned', () => {
mockUseSafeAddress.mockReturnValue(faker.finance.ethereumAddress())
mockUseWallet.mockReturnValue(connectedWalletBuilder().build())

jest.spyOn(ofac, 'useGetIsSanctionedQuery').mockReturnValue({ data: false, refetch: jest.fn() })

const { result } = renderHook(() => useSanctionedAddress())
expect(result.current).toBeUndefined()
})

it('should return safeAddress if it is sanctioned', () => {
const mockSafeAddress = faker.finance.ethereumAddress()
const mockWalletAddress = faker.finance.ethereumAddress()
mockUseSafeAddress.mockReturnValue(mockSafeAddress)
mockUseWallet.mockReturnValue(connectedWalletBuilder().with({ address: mockWalletAddress }).build())

jest
.spyOn(ofac, 'useGetIsSanctionedQuery')
.mockImplementation((address) => ({ data: address === mockSafeAddress, refetch: jest.fn() }))

const { result } = renderHook(() => useSanctionedAddress())
expect(result.current).toEqual(mockSafeAddress)
})

it('should return walletAddress if it is sanctioned', () => {
const mockSafeAddress = faker.finance.ethereumAddress()
const mockWalletAddress = faker.finance.ethereumAddress()
mockUseSafeAddress.mockReturnValue(mockSafeAddress)
mockUseWallet.mockReturnValue(connectedWalletBuilder().with({ address: mockWalletAddress }).build())

jest
.spyOn(ofac, 'useGetIsSanctionedQuery')
.mockImplementation((address) => ({ data: address === mockWalletAddress, refetch: jest.fn() }))

const { result } = renderHook(() => useSanctionedAddress())
expect(result.current).toEqual(mockWalletAddress)
})

it('should return safeAddress if both are sanctioned', () => {
const mockSafeAddress = faker.finance.ethereumAddress()
const mockWalletAddress = faker.finance.ethereumAddress()
mockUseSafeAddress.mockReturnValue(mockSafeAddress)
mockUseWallet.mockReturnValue(connectedWalletBuilder().with({ address: mockWalletAddress }).build())

jest.spyOn(ofac, 'useGetIsSanctionedQuery').mockReturnValue({ data: true, refetch: jest.fn() })

const { result } = renderHook(() => useSanctionedAddress())
expect(result.current).toEqual(mockSafeAddress)
})
})
22 changes: 22 additions & 0 deletions src/hooks/useSanctionedAddress.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { useGetIsSanctionedQuery } from '@/store/ofac'
import useSafeAddress from './useSafeAddress'
import useWallet from './wallets/useWallet'
import { skipToken } from '@reduxjs/toolkit/query/react'

export const useSanctionedAddress = () => {
const wallet = useWallet()
const safeAddress = useSafeAddress()

const { data: isWalletSanctioned } = useGetIsSanctionedQuery(wallet ? wallet.address : skipToken)

const { data: isSafeSanctioned } = useGetIsSanctionedQuery(safeAddress !== '' ? safeAddress : skipToken)

if (isSafeSanctioned) {
return safeAddress
}
if (isWalletSanctioned) {
return wallet?.address
}
Comment on lines +21 to +26
Copy link
Member

Choose a reason for hiding this comment

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

data contains the error message in case the useGetIsSanctionedQuery throws so I think it would falsely say that an address is sanctioned if the network request fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that is not true. The error seems to be transformed and returned in the error field of useGetIsSanctionedQuery

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove the data assignment on error then inside the ofacApi query?

return { error: { status: 'CUSTOM_ERROR', data: (error as Error).message } }


return undefined
}
2 changes: 1 addition & 1 deletion src/store/ofac.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export const ofacApi = createApi({
reducerPath: 'ofacApi',
baseQuery: noopBaseQuery,
endpoints: (builder) => ({
getIsSanctioned: builder.query({
getIsSanctioned: builder.query<boolean, string>({
async queryFn(address, { getState }) {
const state = getState()
const chain = selectChainById(state as RootState, chains.eth)
Expand Down
Loading