Skip to content

Commit

Permalink
Closes #248: Make checkSignatures public (#267)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rmeissner
rmeissner committed Mar 2, 2021
1 parent 1787935 commit b5a9532
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 55 deletions.
16 changes: 6 additions & 10 deletions contracts/GnosisSafe.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ contract GnosisSafe
// Increase nonce and execute transaction.
nonce++;
txHash = keccak256(txHashData);
checkSignatures(txHash, txHashData, signatures, true);
checkSignatures(txHash, txHashData, signatures);
}
// We require some gas to emit the events (at least 2500) after the execution and some to perform code until the execution (500)
// We also include the 1/64 in the check that is not send along with a call to counteract potential shortings because of EIP-150
Expand Down Expand Up @@ -187,10 +187,10 @@ contract GnosisSafe
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
* @param data That should be signed (this is passed to an external validator contract)
* @param signatures Signature data that should be verified. Can be ECDSA signature, contract signature (EIP-1271) or approved hash.
* @param consumeHash Indicates that in case of an approved hash the storage can be freed to save gas
*/
function checkSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures, bool consumeHash)
internal
function checkSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures)
view
public
{
// Load threshold to avoid multiple storage loads
uint256 _threshold = threshold;
Expand Down Expand Up @@ -242,10 +242,6 @@ contract GnosisSafe
currentOwner = address(uint256(r));
// Hashes are automatically approved by the sender of the message or when they have been pre-approved via a separate transaction
require(msg.sender == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "Hash has not been approved");
// Hash has been marked for consumption. If this hash was pre-approved free storage
if (consumeHash && msg.sender != currentOwner) {
approvedHashes[currentOwner][dataHash] = 0;
}
} else if (v > 30) {
// To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
currentOwner = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
Expand Down Expand Up @@ -319,14 +315,14 @@ contract GnosisSafe
*/
function isValidSignature(bytes calldata _data, bytes calldata _signature)
external
view
returns (bytes4)
{
bytes32 messageHash = getMessageHash(_data);
if (_signature.length == 0) {
require(signedMessages[messageHash] != 0, "Hash not approved");
} else {
// consumeHash needs to be false, as the state should not be changed
checkSignatures(messageHash, _data, _signature, false);
checkSignatures(messageHash, _data, _signature);
}
return EIP1271_MAGIC_VALUE;
}
Expand Down
230 changes: 186 additions & 44 deletions test/core/GnosisSafe.Signatures.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { deployments, waffle } from "hardhat";
import "@nomiclabs/hardhat-ethers";
import { AddressZero } from "@ethersproject/constants";
import { getSafeTemplate, getSafeWithOwners } from "../utils/setup";
import { safeSignTypedData, executeTx, safeSignMessage, calculateSafeTransactionHash, safeApproveHash, buildSafeTransaction, logGas, calculateSafeDomainSeparator } from "../utils/execution";
import { safeSignTypedData, executeTx, safeSignMessage, calculateSafeTransactionHash, safeApproveHash, buildSafeTransaction, logGas, calculateSafeDomainSeparator, preimageSafeTransactionHash, buildSignatureBytes } from "../utils/execution";
import { chainId } from "../utils/encoding";

describe("GnosisSafe", async () => {
Expand All @@ -16,11 +16,63 @@ describe("GnosisSafe", async () => {
safe: await getSafeWithOwners([user1.address])
}
})
describe("domainSeparator", async () => {
it('should be correct according to EIP-712', async () => {
const { safe } = await setupTests()
const domainSeparator = calculateSafeDomainSeparator(safe, await chainId())
await expect(
await safe.domainSeparator()
).to.be.eq(domainSeparator)
})
})

describe("checkSignatures", async () => {
describe("getTransactionHash", async () => {
it('should correctly calculate EIP-712 hash', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const typedDataHash = calculateSafeTransactionHash(safe, tx, await chainId())
await expect(
await safe.getTransactionHash(
tx.to, tx.value, tx.data, tx.operation, tx.safeTxGas, tx.baseGas, tx.gasPrice, tx.gasToken, tx.refundReceiver, tx.nonce
)
).to.be.eq(typedDataHash)
})
})

describe("getChainId", async () => {
it('should return correct id', async () => {
const { safe } = await setupTests()
expect(
await safe.getChainId()
).to.be.eq(await chainId())
})
})

describe("approveHash", async () => {
it('approving should only be allowed for owners', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
const signerSafe = safe.connect(user2)
await expect(
signerSafe.approveHash(txHash)
).to.be.revertedWith("Only owners can approve a hash")
})

it('approving should emit event', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
await expect(
safe.approveHash(txHash)
).emit(safe, "ApproveHash").withArgs(txHash, user1.address)
})
})

describe("execTransaction", async () => {
it('should fail if signature points into static part', async () => {
const { safe } = await setupTests()
let signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000020" + "00" + // r, s, v
const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000020" + "00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000000" // Some data to read
await expect(
safe.execTransaction(safe.address, 0, "0x", 0, 0, 0, 0, AddressZero, AddressZero, signatures)
Expand All @@ -30,7 +82,7 @@ describe("GnosisSafe", async () => {
it('should fail if sigantures data is not present', async () => {
const { safe } = await setupTests()

let signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" // r, s, v
const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" // r, s, v

await expect(
safe.execTransaction(safe.address, 0, "0x", 0, 0, 0, 0, AddressZero, AddressZero, signatures)
Expand All @@ -40,33 +92,14 @@ describe("GnosisSafe", async () => {
it('should fail if sigantures data is too short', async () => {
const { safe } = await setupTests()

let signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" + // r, s, v
const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000020" // length

await expect(
safe.execTransaction(safe.address, 0, "0x", 0, 0, 0, 0, AddressZero, AddressZero, signatures)
).to.be.revertedWith("Invalid contract signature location: data not complete")
})

it('should correctly calculate EIP-712 domain separator', async () => {
const { safe } = await setupTests()
const domainSeparator = calculateSafeDomainSeparator(safe, await chainId())
await expect(
await safe.domainSeparator()
).to.be.eq(domainSeparator)
})

it('should correctly calculate EIP-712 hash', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const typedDataHash = calculateSafeTransactionHash(safe, tx, await chainId())
await expect(
await safe.getTransactionHash(
tx.to, tx.value, tx.data, tx.operation, tx.safeTxGas, tx.baseGas, tx.gasPrice, tx.gasToken, tx.refundReceiver, tx.nonce
)
).to.be.eq(typedDataHash)
})

it('should be able to use EIP-712 for signature generation', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
Expand Down Expand Up @@ -118,35 +151,25 @@ describe("GnosisSafe", async () => {
).to.be.revertedWith("Hash has not been approved")
})

it('approving should only be allowed for owners', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
const signerSafe = safe.connect(user2)
await expect(
signerSafe.approveHash(txHash)
).to.be.revertedWith("Only owners can approve a hash")
})

it('approving should emit event', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
await expect(
safe.approveHash(txHash)
).emit(safe, "ApproveHash").withArgs(txHash, user1.address)
})

it('should be able to use pre approved hashes for signature generation', async () => {
const { safe } = await setupTests()
const user2Safe = safe.connect(user2)
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
const approveHashSig = await safeApproveHash(user1, safe, tx)
expect(
await safe.approvedHashes(user1.address, txHash)
).to.be.eq(1)
await expect(
logGas(
"With pre approved signature",
executeTx(user2Safe, tx, [await safeApproveHash(user1, safe, tx)])
executeTx(user2Safe, tx, [approveHashSig])
)
).to.emit(safe, "ExecutionSuccess")
// Approved hash should not reset automatically
expect(
await safe.approvedHashes(user1.address, txHash)
).to.be.eq(1)
})

it('should revert if threshold is not set', async () => {
Expand Down Expand Up @@ -193,4 +216,123 @@ describe("GnosisSafe", async () => {
).to.emit(safe, "ExecutionSuccess")
})
})

describe("checkSignatures", async () => {
it('should fail if signature points into static part', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000020" + "00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000000" // Some data to read
await expect(
safe.checkSignatures(txHash, txHashData, signatures)
).to.be.revertedWith("Invalid contract signature location: inside static part")
})

it('should fail if sigantures data is not present', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())

const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" // r, s, v

await expect(
safe.checkSignatures(txHash, txHashData, signatures)
).to.be.revertedWith("Invalid contract signature location: length not present")
})

it('should fail if sigantures data is too short', async () => {
const { safe } = await setupTests()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())

const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000020" // length

await expect(
safe.checkSignatures(txHash, txHashData, signatures)
).to.be.revertedWith("Invalid contract signature location: data not complete")
})

it('should not be able to use different chainId for signing', async () => {
await setupTests()
const safe = await getSafeWithOwners([user1.address])
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
const signatures = buildSignatureBytes([await safeSignTypedData(user1, safe, tx, 1)])
await expect(
safe.checkSignatures(txHash, txHashData, signatures)
).to.be.revertedWith("Invalid owner provided")
})

it('if not msg.sender on-chain approval is required', async () => {
const { safe } = await setupTests()
const user2Safe = safe.connect(user2)
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)])
await expect(
user2Safe.checkSignatures(txHash, txHashData, signatures)
).to.be.revertedWith("Hash has not been approved")
})

it('should revert if threshold is not set', async () => {
await setupTests()
const safe = await getSafeTemplate()
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
await expect(
safe.checkSignatures(txHash, txHashData, "0x")
).to.be.revertedWith("Threshold needs to be defined!")
})

it('should revert if not the required amount of signature data is provided', async () => {
await setupTests()
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address])
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
await expect(
safe.checkSignatures(txHash, txHashData, "0x")
).to.be.revertedWith("Signatures data too short")
})

it('should not be able to use different signature type of same owner', async () => {
await setupTests()
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address])
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
const signatures = buildSignatureBytes([
await safeApproveHash(user1, safe, tx),
await safeSignTypedData(user1, safe, tx),
await safeSignTypedData(user3, safe, tx)
])
await expect(
safe.checkSignatures(txHash, txHashData, signatures)
).to.be.revertedWith("Invalid owner provided")
})

it('should be able to mix all signature types', async () => {
await setupTests()
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address])
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
const signatures = buildSignatureBytes([
await safeApproveHash(user1, safe, tx, true),
await safeApproveHash(user4, safe, tx),
await safeSignTypedData(user2, safe, tx),
await safeSignTypedData(user3, safe, tx)
])

await safe.checkSignatures(txHash, txHashData, signatures)
})
})
})
2 changes: 1 addition & 1 deletion test/libraries/MultiSend.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe("MultiSend", async () => {
const source = `
contract Test {
function killme() public {
selfdestruct(msg.sender);
selfdestruct(payable(msg.sender));
}
}`
const killLib = await deployContract(user1, source);
Expand Down
4 changes: 4 additions & 0 deletions test/utils/execution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ export const calculateSafeDomainSeparator = (safe: Contract, chainId: BigNumberi
return utils._TypedDataEncoder.hashDomain({ verifyingContract: safe.address, chainId })
}

export const preimageSafeTransactionHash = (safe: Contract, safeTx: SafeTransaction, chainId: BigNumberish): string => {
return utils._TypedDataEncoder.encode({ verifyingContract: safe.address, chainId }, EIP712_SAFE_TX_TYPE, safeTx)
}

export const calculateSafeTransactionHash = (safe: Contract, safeTx: SafeTransaction, chainId: BigNumberish): string => {
return utils._TypedDataEncoder.hash({ verifyingContract: safe.address, chainId }, EIP712_SAFE_TX_TYPE, safeTx)
}
Expand Down

0 comments on commit b5a9532

Please sign in to comment.