Skip to content

Commit

Permalink
feat: disable blind signing by default (#3605)
Browse files Browse the repository at this point in the history
* feat: disable blind signing by default

* fix: review issues

* fix: remove whitespace, always link
  • Loading branch information
schmanu authored Apr 29, 2024
1 parent dd593da commit 4b4055c
Show file tree
Hide file tree
Showing 8 changed files with 201 additions and 5 deletions.
4 changes: 4 additions & 0 deletions src/components/balances/AssetsTable/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ describe('AssetsTable', () => {
},
signing: {
onChainSigning: false,
blindSigning: false,
},
transactionExecution: true,
},
Expand Down Expand Up @@ -211,6 +212,7 @@ describe('AssetsTable', () => {
},
signing: {
onChainSigning: false,
blindSigning: false,
},
transactionExecution: true,
},
Expand Down Expand Up @@ -312,6 +314,7 @@ describe('AssetsTable', () => {
},
signing: {
onChainSigning: false,
blindSigning: false,
},
transactionExecution: true,
},
Expand Down Expand Up @@ -410,6 +413,7 @@ describe('AssetsTable', () => {
},
signing: {
onChainSigning: false,
blindSigning: false,
},
transactionExecution: true,
},
Expand Down
1 change: 1 addition & 0 deletions src/components/balances/HiddenTokenButton/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ describe('HiddenTokenToggle', () => {
},
signing: {
onChainSigning: false,
blindSigning: false,
},
transactionExecution: true,
},
Expand Down
3 changes: 3 additions & 0 deletions src/components/settings/SecurityLogin/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { isSocialLoginWallet } from '@/services/mpc/SocialLoginModule'
import SpendingLimits from '../SpendingLimits'
import dynamic from 'next/dynamic'
import { useIsRecoverySupported } from '@/features/recovery/hooks/useIsRecoverySupported'
import SecuritySettings from '../SecuritySettings'

const RecoverySettings = dynamic(() => import('@/features/recovery/components/RecoverySettings'))

Expand Down Expand Up @@ -49,6 +50,8 @@ const SecurityLogin = () => {
)}

<SpendingLimits />

<SecuritySettings />
</Box>
)
}
Expand Down
40 changes: 40 additions & 0 deletions src/components/settings/SecuritySettings/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { useAppDispatch, useAppSelector } from '@/store'
import { selectBlindSigning, setBlindSigning } from '@/store/settingsSlice'
import { Paper, Grid, Typography, FormGroup, FormControlLabel, Checkbox } from '@mui/material'

const SecuritySettings = () => {
const isBlindSigningEnabled = useAppSelector(selectBlindSigning)
const dispatch = useAppDispatch()

return (
<Paper sx={{ padding: 4 }}>
<Grid container spacing={3}>
<Grid item lg={4} xs={12}>
<Typography variant="h4" fontWeight="bold" mb={1}>
Security
</Typography>
</Grid>

<Grid item xs>
<Typography mb={2}>
Enabling this setting allows the signing of unreadable signature requests. Signing these messages can lead
to unpredictable consequences, including the potential loss of funds or control over your account.
</Typography>
<FormGroup>
<FormControlLabel
control={
<Checkbox
checked={isBlindSigningEnabled}
onChange={() => dispatch(setBlindSigning(!isBlindSigningEnabled))}
/>
}
label="Enable blind signing"
/>
</FormGroup>
</Grid>
</Grid>
</Paper>
)
}

export default SecuritySettings
53 changes: 50 additions & 3 deletions src/components/tx-flow/flows/SignMessage/SignMessage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
Accordion,
AccordionSummary,
AccordionDetails,
Link,
} from '@mui/material'
import { useTheme } from '@mui/material/styles'
import ExpandMoreIcon from '@mui/icons-material/ExpandMore'
Expand Down Expand Up @@ -44,10 +45,14 @@ import { SafeTxContext } from '../../SafeTxProvider'
import RiskConfirmationError from '@/components/tx/SignOrExecuteForm/RiskConfirmationError'
import { Redefine } from '@/components/tx/security/redefine'
import { TxSecurityContext } from '@/components/tx/security/shared/TxSecurityContext'
import { isEIP712TypedData } from '@/utils/safe-messages'
import { isBlindSigningPayload, isEIP712TypedData } from '@/utils/safe-messages'
import ApprovalEditor from '@/components/tx/ApprovalEditor'
import { ErrorBoundary } from '@sentry/react'
import { isWalletRejection } from '@/utils/wallets'
import { useAppSelector } from '@/store'
import { selectBlindSigning } from '@/store/settingsSlice'
import NextLink from 'next/link'
import { AppRoutes } from '@/config/routes'

const createSkeletonMessage = (confirmationsRequired: number): SafeMessage => {
return {
Expand Down Expand Up @@ -143,6 +148,40 @@ const AlreadySignedByOwnerMessage = ({ hasSigned }: { hasSigned: boolean }) => {
)
}

const BlindSigningWarning = ({
isBlindSigningEnabled,
isBlindSigningPayload,
}: {
isBlindSigningEnabled: boolean
isBlindSigningPayload: boolean
}) => {
if (!isBlindSigningPayload) {
return null
}

return (
<ErrorMessage level={isBlindSigningEnabled ? 'warning' : 'error'}>
This request involves{' '}
<Link component={NextLink} href={AppRoutes.settings.securityLogin}>
blind signing
</Link>
, which can lead to unpredictable outcomes.
<br />
{isBlindSigningEnabled ? (
'Proceed with caution.'
) : (
<>
If you wish to proceed, you must first{' '}
<Link component={NextLink} href={AppRoutes.settings.securityLogin}>
enable blind signing
</Link>
.
</>
)}
</ErrorMessage>
)
}

const SuccessCard = ({ safeMessage, onContinue }: { safeMessage: SafeMessage; onContinue: () => void }) => {
return (
<TxCard>
Expand Down Expand Up @@ -191,7 +230,10 @@ const SignMessage = ({ message, safeAppId, requestId }: ProposeProps | ConfirmPr
const decodedMessageAsString = isPlainTextMessage ? decodedMessage : JSON.stringify(decodedMessage, null, 2)
const hasSigned = !!safeMessage?.confirmations.some(({ owner }) => owner.value === wallet?.address)
const isFullySigned = !!safeMessage?.preparedSignature
const isDisabled = !isOwner || hasSigned || !safe.deployed
const isEip712 = isEIP712TypedData(decodedMessage)
const isBlindSigningRequest = isBlindSigningPayload(decodedMessage)
const isBlindSigningEnabled = useAppSelector(selectBlindSigning)
const isDisabled = !isOwner || hasSigned || !safe.deployed || (!isBlindSigningEnabled && isBlindSigningRequest)

const { onSign, submitError } = useSyncSafeMessageSigner(
safeMessage,
Expand Down Expand Up @@ -239,12 +281,17 @@ const SignMessage = ({ message, safeAppId, requestId }: ProposeProps | ConfirmPr
<CardContent>
<DialogHeader threshold={safe.threshold} />

{isEIP712TypedData(decodedMessage) && (
{isEip712 && (
<ErrorBoundary fallback={<div>Error parsing data</div>}>
<ApprovalEditor safeMessage={decodedMessage} />
</ErrorBoundary>
)}

<BlindSigningWarning
isBlindSigningEnabled={isBlindSigningEnabled}
isBlindSigningPayload={isBlindSigningRequest}
/>

<Typography fontWeight={700} mt={2} mb={1}>
Message: <CopyButton text={decodedMessageAsString} />
</Typography>
Expand Down
7 changes: 7 additions & 0 deletions src/store/settingsSlice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export type SettingsState = {
env: EnvState
signing: {
onChainSigning: boolean
blindSigning: boolean
}
transactionExecution: boolean
}
Expand Down Expand Up @@ -68,6 +69,7 @@ export const initialState: SettingsState = {
},
signing: {
onChainSigning: false,
blindSigning: false,
},
transactionExecution: true,
}
Expand Down Expand Up @@ -115,6 +117,9 @@ export const settingsSlice = createSlice({
setOnChainSigning: (state, { payload }: PayloadAction<boolean>) => {
state.signing.onChainSigning = payload
},
setBlindSigning: (state, { payload }: PayloadAction<boolean>) => {
state.signing.blindSigning = payload
},
setSettings: (_, { payload }: PayloadAction<SettingsState>) => {
// We must return as we are overwriting the entire state
// Preserve default nested settings if importing without
Expand All @@ -135,6 +140,7 @@ export const {
setTenderly,
setOnChainSigning,
setTransactionExecution,
setBlindSigning,
} = settingsSlice.actions

export const selectSettings = (state: RootState): SettingsState => state[settingsSlice.name]
Expand Down Expand Up @@ -163,3 +169,4 @@ export const isEnvInitialState = createSelector([selectSettings, (_, chainId) =>
})

export const selectOnChainSigning = createSelector(selectSettings, (settings) => settings.signing.onChainSigning)
export const selectBlindSigning = createSelector(selectSettings, (settings) => settings.signing.blindSigning)
92 changes: 91 additions & 1 deletion src/utils/__tests__/safe-messages.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { zeroPadValue } from 'ethers'
import type { SafeInfo } from '@safe-global/safe-gateway-typescript-sdk'

import { generateSafeMessageTypedData, isOffchainEIP1271Supported } from '../safe-messages'
import { generateSafeMessageTypedData, isBlindSigningPayload, isOffchainEIP1271Supported } from '../safe-messages'
import { toBeHex } from 'ethers'
import { FEATURES } from '../chains'
import { faker } from '@faker-js/faker'

const MOCK_ADDRESS = zeroPadValue('0x0123', 20)

Expand Down Expand Up @@ -352,4 +353,93 @@ describe('safe-messages', () => {
).toBeTruthy()
})
})

describe('isBlindSigningPayload', () => {
it('should detect blind signing requests', () => {
expect(isBlindSigningPayload(`0x${faker.number.hex()}`)).toBeTruthy()
expect(isBlindSigningPayload(`0x${faker.number.hex()}`)).toBeTruthy()
expect(isBlindSigningPayload(`0x${faker.number.hex()}`)).toBeTruthy()
expect(isBlindSigningPayload(`0x${faker.number.hex()}`)).toBeTruthy()
expect(isBlindSigningPayload(`0x${faker.number.hex()}`)).toBeTruthy()
})

it('should not flag EIP 712 messages', () => {
const message = {
domain: {
chainId: 1,
name: 'Ether Mail',
verifyingContract: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC',
version: '1',
},
message: {
contents: 'Hello, Bob!',
from: {
name: 'Cow',
wallet: '0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826',
},
to: {
name: 'Bob',
wallet: '0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB',
},
},
primaryType: 'Mail',
types: {
EIP712Domain: [
{
name: 'name',
type: 'string',
},
{
name: 'version',
type: 'string',
},
{
name: 'chainId',
type: 'uint256',
},
{
name: 'verifyingContract',
type: 'address',
},
],
Mail: [
{
name: 'from',
type: 'Person',
},
{
name: 'to',
type: 'Person',
},
{
name: 'contents',
type: 'string',
},
],
Person: [
{
name: 'name',
type: 'string',
},
{
name: 'wallet',
type: 'address',
},
],
},
}
expect(isBlindSigningPayload(message)).toBeFalsy()
})

it('should not flag legit message requests', () => {
expect(
isBlindSigningPayload(
'localhost wants you to sign in with your Ethereum account: 0x6Ee9894c677EFa1c56392e5E7533DE76004C8D94\n\nThis is a test statement.\n\nURI: https://localhost/login\nVersion: 1\nChain ID: 1\nNonce: oNCEHm5jzQU2WvuBB\nIssued At: 2022-01-28T23:28:16.013Z',
),
).toBeFalsy()

expect(isBlindSigningPayload('I hereby confirm order 0x123456ABC')).toBeFalsy()
expect(isBlindSigningPayload('0x432165 should be invalidated')).toBeFalsy()
})
})
})
6 changes: 5 additions & 1 deletion src/utils/safe-messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,18 @@ const EIP1271_SUPPORTED_SAFE_VERSION = '1.0.0'

const EIP1271_OFFCHAIN_SUPPORTED_SAFE_APPS_SDK_VERSION = '7.11.0'

/**
const isHash = (payload: string) => /^0x[a-f0-9]+$/i.test(payload)

/*
* Typeguard for EIP712TypedData
*
*/
export const isEIP712TypedData = (obj: any): obj is EIP712TypedData => {
return typeof obj === 'object' && obj != null && 'domain' in obj && 'types' in obj && 'message' in obj
}

export const isBlindSigningPayload = (obj: EIP712TypedData | string): boolean => !isEIP712TypedData(obj) && isHash(obj)

export const generateSafeMessageMessage = (message: SafeMessage['message']): string => {
return typeof message === 'string' ? hashMessage(message) : hashTypedData(message)
}
Expand Down

0 comments on commit 4b4055c

Please sign in to comment.