Skip to content

Implement contract signature verification #238

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

Merged
merged 2 commits into from
Feb 3, 2025
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"@graphql-yoga/plugin-response-cache": "^3.5.0",
"@hypercerts-org/contracts": "2.0.0-alpha.12",
"@hypercerts-org/marketplace-sdk": "0.3.37",
"@hypercerts-org/sdk": "2.5.0-beta.3",
"@hypercerts-org/sdk": "2.5.0-beta.6",
"@ipld/car": "^5.2.5",
"@openzeppelin/merkle-tree": "^1.0.5",
"@safe-global/api-kit": "^2.5.4",
Expand Down
10 changes: 5 additions & 5 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 6 additions & 9 deletions src/lib/safe/signature-verification/SafeSignatureVerifier.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import Safe from "@safe-global/protocol-kit";
import { getAddress, hashTypedData, type HashTypedDataParameters } from "viem";

import { EvmClientFactory } from "../../../client/evmClient.js";

import { RpcStrategyFactory } from "../safe-rpc-urls.js";
import { SignatureVerifierStrategyFactory } from "./SignatureVerifierStrategy.js";

export default abstract class SafeSignatureVerifier {
protected chainId: number;
Expand Down Expand Up @@ -35,13 +35,10 @@ export default abstract class SafeSignatureVerifier {

abstract buildTypedData(): Omit<HashTypedDataParameters, "domain">;

async verify(signature: string): Promise<boolean> {
const safe = await Safe.default.init({
provider: this.rpcUrl,
safeAddress: this.safeAddress,
});

const protocolKit = await safe.connect({});
return protocolKit.isValidSignature(this.hashTypedData(), signature);
async verify(signature: `0x${string}`): Promise<boolean> {
return SignatureVerifierStrategyFactory.getStrategy(
this.chainId,
this.hashTypedData(),
).verify(signature, this.rpcUrl, this.safeAddress);
}
}
83 changes: 83 additions & 0 deletions src/lib/safe/signature-verification/SignatureVerifierStrategy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import Safe from "@safe-global/protocol-kit";
import { ethers } from "ethers";

const EIP1271_MAGIC_VALUE =
"0x1626ba7e00000000000000000000000000000000000000000000000000000000";

export interface SignatureVerifierStrategy {
verify(
signature: `0x${string}`,
rpcUrl: string,
safeAddress: `0x${string}`,
): Promise<boolean>;
}

export class SignatureVerifierStrategyFactory {
static getStrategy(
Copy link
Member

Choose a reason for hiding this comment

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

Since choosing the strategy isn't influenced by the hash of the typed data, I feel like it shouldn't be here. It also doesn't necessarily need to be on the constructor if the other fields aren't either. From the viewpoint of the individual classes you can't necessarily say that the hash is a value that you would want to keep across multiple invocations, so the actual call to verify() seems like it's lacking this value.

It's absolutely fine the way it is, but would probably be something that I'd refactor given I have the time and am working on something that's touching this code :)

chainId: number,
hashTypedData: `0x${string}`,
): SignatureVerifierStrategy {
switch (chainId) {
case 314:
case 314159:
return new ContractSignatureVerifierStrategy(hashTypedData);
default:
return new SafeSignatureVerifierStrategy(hashTypedData);
}
}
}

export class SafeSignatureVerifierStrategy
implements SignatureVerifierStrategy
{
private hashTypedData: `0x${string}`;
constructor(hashTypedData: `0x${string}`) {
this.hashTypedData = hashTypedData;
}

async verify(
signature: `0x${string}`,
rpcUrl: string,
safeAddress: `0x${string}`,
): Promise<boolean> {
const safe = await Safe.default.init({
provider: rpcUrl,
safeAddress: safeAddress,
});

const protocolKit = await safe.connect({});
return protocolKit.isValidSignature(this.hashTypedData, signature);
}
}

export class ContractSignatureVerifierStrategy
implements SignatureVerifierStrategy
{
private hashTypedData: `0x${string}`;
constructor(hashTypedData: `0x${string}`) {
this.hashTypedData = hashTypedData;
}

async verify(
signature: `0x${string}`,
rpcUrl: string,
safeAddress: `0x${string}`,
): Promise<boolean> {
const provider = new ethers.JsonRpcProvider(rpcUrl);
const iface = new ethers.Interface([
"function isValidSignature(bytes32 hash, bytes signature) view returns (bytes4)",
]);
const calldata = iface.encodeFunctionData("isValidSignature", [
this.hashTypedData,
signature,
]);

try {
const result = await provider.call({ to: safeAddress, data: calldata });
return result === EIP1271_MAGIC_VALUE;
} catch (error) {
console.error("Error:", error);
return false;
}
}
}
2 changes: 1 addition & 1 deletion vitest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export default defineConfig({
thresholds: {
lines: 25,
branches: 72,
functions: 63,
functions: 61,
statements: 25,
},
include: ["src/**/*.ts"],
Expand Down