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

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
5 changes: 4 additions & 1 deletion src/components/common/SafeTokenWidget/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import SafeTokenIcon from '@/public/images/common/safe-token.svg'
import css from './styles.module.css'
import UnreadBadge from '../UnreadBadge'
import classnames from 'classnames'
import { useSanctionedAddress } from '@/hooks/useSanctionedAddress'

const TOKEN_DECIMALS = 18

Expand Down Expand Up @@ -47,8 +48,10 @@ const SafeTokenWidget = () => {
const [allocationData, , allocationDataLoading] = useSafeTokenAllocation()
const [allocation, , allocationLoading] = useSafeVotingPower(allocationData)

const sanctionedAddress = useSanctionedAddress()

const tokenAddress = getSafeTokenAddress(chainId)
if (!tokenAddress) {
if (!tokenAddress || Boolean(sanctionedAddress)) {
return null
}

Expand Down
6 changes: 5 additions & 1 deletion src/components/dashboard/ActivityRewardsSection/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { useHasFeature } from '@/hooks/useChains'
import { FEATURES } from '@/utils/chains'
import useLocalStorage from '@/services/local-storage/useLocalStorage'
import ExternalLink from '@/components/common/ExternalLink'
import { useSanctionedAddress } from '@/hooks/useSanctionedAddress'

const Step = ({ active, title }: { active: boolean; title: ReactNode }) => {
return (
Expand Down Expand Up @@ -43,12 +44,15 @@ const ActivityRewardsSection = () => {
const isDarkMode = useDarkMode()
const router = useRouter()

const sanctionedAddress = useSanctionedAddress()

const [widgetHidden = false, setWidgetHidden] = useLocalStorage<boolean>(LOCAL_STORAGE_KEY_HIDE_WIDGET)

const isSAPBannerEnabled = useHasFeature(FEATURES.SAP_BANNER)
const governanceApp = matchingApps?.[0]

if (!governanceApp || !governanceApp?.url || !isSAPBannerEnabled || widgetHidden) return null
if (!governanceApp || !governanceApp?.url || !isSAPBannerEnabled || widgetHidden || Boolean(sanctionedAddress))
return null

const appUrl = getSafeAppUrl(router, governanceApp?.url)

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?

}

// 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
}
4 changes: 2 additions & 2 deletions 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 All @@ -59,7 +59,7 @@ export const ofacApi = createApi({
const isAddressBlocked: boolean = await contract['isSanctioned'](address)
return { data: isAddressBlocked }
} catch (error) {
return { error: { status: 'CUSTOM_ERROR', data: (error as Error).message } }
return { error }
}
},
}),
Expand Down
Loading