diff --git a/contracts/GnosisSafe.sol b/contracts/GnosisSafe.sol index 5aa4989ba..31aefa798 100644 --- a/contracts/GnosisSafe.sol +++ b/contracts/GnosisSafe.sol @@ -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 @@ -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; @@ -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); @@ -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; } diff --git a/test/core/GnosisSafe.Signatures.spec.ts b/test/core/GnosisSafe.Signatures.spec.ts index f8f172a0f..0e6b14f33 100644 --- a/test/core/GnosisSafe.Signatures.spec.ts +++ b/test/core/GnosisSafe.Signatures.spec.ts @@ -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 () => { @@ -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) @@ -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) @@ -40,7 +92,7 @@ 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( @@ -48,25 +100,6 @@ describe("GnosisSafe", async () => { ).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() }) @@ -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 () => { @@ -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) + }) + }) }) \ No newline at end of file diff --git a/test/libraries/MultiSend.spec.ts b/test/libraries/MultiSend.spec.ts index 2535aa125..748b2c386 100644 --- a/test/libraries/MultiSend.spec.ts +++ b/test/libraries/MultiSend.spec.ts @@ -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); diff --git a/test/utils/execution.ts b/test/utils/execution.ts index 1e3f8597b..dcfa13a32 100644 --- a/test/utils/execution.ts +++ b/test/utils/execution.ts @@ -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) }