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

walletconnect: sanitize 712 signs #5159

Merged
merged 4 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
15 changes: 13 additions & 2 deletions src/components/transaction/TransactionMessage.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { Text } from '../text';
import styled from '@/styled-thing';
import { padding } from '@/styles';
import { deviceUtils } from '@/utils';
import { sanitizeTypedData } from '@/utils/signingUtils';
import { RainbowError, logger } from '@/logger';

const deviceWidth = deviceUtils.dimensions.width;
const horizontalPadding = 24;
Expand Down Expand Up @@ -34,9 +36,18 @@ const TransactionMessage = ({ maxHeight = 150, message, method }) => {
maximumHeight = 200;
minimumHeight = 200;
try {
msg = JSON.parse(message);
const parsedMessage = JSON.parse(message);
const sanitizedMessage = sanitizeTypedData(parsedMessage);
msg = sanitizedMessage;
// eslint-disable-next-line no-empty
} catch (e) {}
} catch (e) {
logger.error(
new RainbowError('TransactionMessage: Error parsing message ', {
messageType: typeof message,
})
);
}

msg = JSON.stringify(msg, null, 4);
}

Expand Down
8 changes: 7 additions & 1 deletion src/model/wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ import {
import { DebugContext } from '@/logger/debugContext';
import { IS_ANDROID } from '@/env';
import { setHardwareTXError } from '@/navigation/HardwareWalletTxNavigator';
import { sanitizeTypedData } from '@/utils/signingUtils';

export type EthereumPrivateKey = string;
type EthereumMnemonic = string;
Expand Down Expand Up @@ -517,8 +518,13 @@ export const signTypedDataMessage = async (
}
try {
let parsedData = message;

// we need to parse the data different for both possible types
try {
parsedData = typeof message === 'string' && JSON.parse(message);
parsedData =
typeof message === 'string'
? sanitizeTypedData(JSON.parse(message))
: sanitizeTypedData(message);
// eslint-disable-next-line no-empty
} catch (e) {}

Expand Down
35 changes: 35 additions & 0 deletions src/utils/signingUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// This function removes all the keys from the message that are not present in the types
// preventing a know phising attack where the signature process could allow malicious DApps
// to trick users into signing an EIP-712 object different from the one presented
// in the signature approval preview. Consequently, users were at risk of unknowingly
// transferring control of their ERC-20 tokens, NFTs, etc to adversaries by signing
// hidden Permit messages.

// For more info read https://www.coinspect.com/wallet-EIP-712-injection-vulnerability/

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const sanitizeTypedData = (data: any) => {
if (data.types[data.primaryType].length > 0) {
// Extract all the valid permit types for the primary type
const permitPrimaryTypes: string[] = data.types[data.primaryType].map(
(type: { name: string; type: string }) => type.name
);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const sanitizedMessage: any = {};
// Extract all the message keys that matches the valid permit types
Object.keys(data.message).forEach(key => {
if (permitPrimaryTypes.includes(key)) {
sanitizedMessage[key] = data.message[key];
}
});

const sanitizedData = {
...data,
message: sanitizedMessage,
};

return sanitizedData;
}
return data;
};