diff --git a/.husky/.gitignore b/.husky/.gitignore new file mode 100644 index 000000000..31354ec13 --- /dev/null +++ b/.husky/.gitignore @@ -0,0 +1 @@ +_ diff --git a/.husky/pre-commit b/.husky/pre-commit new file mode 100755 index 000000000..161cd32db --- /dev/null +++ b/.husky/pre-commit @@ -0,0 +1,21 @@ +#!/bin/sh +. "$(dirname "$0")/_/husky.sh" + +# Redirect output to stderr. +exec 1>&2 + +# prevent it.only or describe.only commited +if [ "$allowonlytests" != "true" ] && + test $(git diff --cached | grep -E "\b(it|describe).only\(" | wc -l) != 0 +then + cat <<\EOF +Error: Attempt to add it.only or describe.only - which may disable all other tests + +If you know what you are doing you can disable this check using: + + git config hooks.allowonlytests true +EOF + exit 1 +fi + +exit 0 diff --git a/contracts/GnosisSafe.sol b/contracts/GnosisSafe.sol index 31aefa798..dd904b251 100644 --- a/contracts/GnosisSafe.sol +++ b/contracts/GnosisSafe.sol @@ -5,6 +5,7 @@ import "./base/FallbackManager.sol"; import "./common/MasterCopy.sol"; import "./common/SignatureDecoder.sol"; import "./common/SecuredTokenTransfer.sol"; +import "./common/StorageAccessible.sol"; import "./interfaces/ISignatureValidator.sol"; import "./external/GnosisSafeMath.sol"; @@ -13,7 +14,7 @@ import "./external/GnosisSafeMath.sol"; /// @author Richard Meissner - /// @author Ricardo Guilherme Schmidt - (Status Research & Development GmbH) - Gas Token Payment contract GnosisSafe - is MasterCopy, ModuleManager, OwnerManager, SignatureDecoder, SecuredTokenTransfer, ISignatureValidatorConstants, FallbackManager { + is MasterCopy, ModuleManager, OwnerManager, SignatureDecoder, SecuredTokenTransfer, ISignatureValidatorConstants, FallbackManager, StorageAccessible { using GnosisSafeMath for uint256; @@ -265,6 +266,7 @@ contract GnosisSafe /// @param data Data payload of Safe transaction. /// @param operation Operation type of Safe transaction. /// @return Estimate without refunds and overhead fees (base transaction and payload data gas costs). + /// @notice Deprecated in favor of common/StorageAccessible.sol and will be removed in next version. function requiredTxGas(address to, uint256 value, bytes calldata data, Enum.Operation operation) external returns (uint256) diff --git a/contracts/accessors/SimulateTxAccessor.sol b/contracts/accessors/SimulateTxAccessor.sol new file mode 100644 index 000000000..bf1d95e89 --- /dev/null +++ b/contracts/accessors/SimulateTxAccessor.sol @@ -0,0 +1,44 @@ +pragma solidity >=0.5.0 <0.6.0; + +import "../base/Executor.sol"; + +/// @title Simulate Transaction Accessor - can be used with StorageAccessible to simulate Safe transactions +/// @author Richard Meissner - +contract SimulateTxAccessor is Executor { + + bytes32 constant private GUARD_VALUE = keccak256("simulate_tx_accessor.guard.bytes32"); + bytes32 guard; + + constructor() public { + guard = GUARD_VALUE; + } + + modifier onlyDelegateCall() { + require(guard != GUARD_VALUE, "SimulateTxAccessor should only be called via delegatecall"); + _; + } + + function simulate(address to, uint256 value, bytes calldata data, Enum.Operation operation) + external + onlyDelegateCall() + returns(uint256 estimate, bool success, bytes memory returnData) + { + uint256 startGas = gasleft(); + success = execute(to, value, data, operation, gasleft()); + estimate = startGas - gasleft(); + // solium-disable-next-line security/no-inline-assembly + assembly { + // Load free memory location + let ptr := mload(0x40) + // We allocate memory for the return data by setting the free memory location to + // current free memory location + data size + 32 bytes for data size value + mstore(0x40, add(ptr, add(returndatasize(), 0x20))) + // Store the size + mstore(ptr, returndatasize()) + // Store the data + returndatacopy(add(ptr, 0x20), 0, returndatasize()) + // Point the return data to the correct memory location + returnData := ptr + } + } +} \ No newline at end of file diff --git a/contracts/common/SignatureDecoder.sol b/contracts/common/SignatureDecoder.sol index 3cf7b5007..e13a252e3 100644 --- a/contracts/common/SignatureDecoder.sol +++ b/contracts/common/SignatureDecoder.sol @@ -2,7 +2,6 @@ pragma solidity >=0.5.0 <0.6.0; /// @title SignatureDecoder - Decodes signatures that a encoded as bytes -/// @author Ricardo Guilherme Schmidt (Status Research & Development GmbH) /// @author Richard Meissner - contract SignatureDecoder { diff --git a/contracts/common/StorageAccessible.sol b/contracts/common/StorageAccessible.sol new file mode 100644 index 000000000..5202d49c7 --- /dev/null +++ b/contracts/common/StorageAccessible.sol @@ -0,0 +1,86 @@ +pragma solidity >=0.5.0 <0.6.0; + +/// @title StorageAccessible - generic base contract that allows callers to access all internal storage. +/// @notice Adjusted version of https://github.com/gnosis/util-contracts/blob/3db1e531cb243a48ea91c60a800d537c1000612a/contracts/StorageAccessible.sol +contract StorageAccessible { + bytes4 internal constant SIMULATE_DELEGATECALL_INTERNAL_SELECTOR = bytes4( + keccak256("simulateDelegatecallInternal(address,bytes)") + ); + + /** + * @dev Reads `length` bytes of storage in the currents contract + * @param offset - the offset in the current contract's storage in words to start reading from + * @param length - the number of words (32 bytes) of data to read + * @return the bytes that were read. + */ + function getStorageAt(uint256 offset, uint256 length) + public + view + returns (bytes memory) + { + bytes memory result = new bytes(length * 32); + for (uint256 index = 0; index < length; index++) { + assembly { + let word := sload(add(offset, index)) + mstore(add(add(result, 0x20), mul(index, 0x20)), word) + } + } + return result; + } + + /** + * @dev Performs a delegetecall on a targetContract in the context of self. + * Internally reverts execution to avoid side effects (making it static). Catches revert and returns encoded result as bytes. + * @param targetContract Address of the contract containing the code to execute. + * @param calldataPayload Calldata that should be sent to the target contract (encoded method name and arguments). + */ + function simulateDelegatecall( + address targetContract, + bytes memory calldataPayload + ) public returns (bytes memory) { + bytes memory innerCall = abi.encodeWithSelector( + SIMULATE_DELEGATECALL_INTERNAL_SELECTOR, + targetContract, + calldataPayload + ); + (, bytes memory response) = address(this).call(innerCall); + bool innerSuccess = response[response.length - 1] == 0x01; + setLength(response, response.length - 1); + if (innerSuccess) { + return response; + } else { + revertWith(response); + } + } + + /** + * @dev Performs a delegetecall on a targetContract in the context of self. + * Internally reverts execution to avoid side effects (making it static). Returns encoded result as revert message + * concatenated with the success flag of the inner call as a last byte. + * @param targetContract Address of the contract containing the code to execute. + * @param calldataPayload Calldata that should be sent to the target contract (encoded method name and arguments). + */ + function simulateDelegatecallInternal( + address targetContract, + bytes memory calldataPayload + ) public returns (bytes memory) { + (bool success, bytes memory response) = targetContract.delegatecall( + calldataPayload + ); + revertWith(abi.encodePacked(response, success)); + } + + function revertWith(bytes memory response) internal pure { + // solium-disable-next-line security/no-inline-assembly + assembly { + revert(add(response, 0x20), mload(response)) + } + } + + function setLength(bytes memory buffer, uint256 length) internal pure { + // solium-disable-next-line security/no-inline-assembly + assembly { + mstore(buffer, length) + } + } +} \ No newline at end of file diff --git a/contracts/interfaces/ViewStorageAccessible.sol b/contracts/interfaces/ViewStorageAccessible.sol new file mode 100644 index 000000000..353a8ff2f --- /dev/null +++ b/contracts/interfaces/ViewStorageAccessible.sol @@ -0,0 +1,14 @@ +pragma solidity >=0.5.0 <0.6.0; + +/// @title ViewStorageAccessible - Interface on top of StorageAccessible base class to allow simulations from view functions +/// @notice Adjusted version of https://github.com/gnosis/util-contracts/blob/3db1e531cb243a48ea91c60a800d537c1000612a/contracts/StorageAccessible.sol +interface ViewStorageAccessible { + /** + * @dev Same as `simulateDelegatecall` on StorageAccessible. Marked as view so that it can be called from external contracts + * that want to run simulations from within view functions. Will revert if the invoked simulation attempts to change state. + */ + function simulateDelegatecall( + address targetContract, + bytes calldata calldataPayload + ) external view returns (bytes memory); +} \ No newline at end of file diff --git a/package.json b/package.json index b66236713..17a6d4991 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,8 @@ "lint:sol": "solhint 'contracts/**/*.sol'", "lint:ts": "eslint --max-warnings 0 .", "fmt:sol": "prettier 'contracts/**/*.sol' -w", - "prepack": "yarn build" + "prepack": "yarn build", + "prepare": "husky install" }, "repository": { "type": "git", @@ -58,6 +59,7 @@ "ethereum-waffle": "^3.2.0", "hardhat": "^2.0.8", "hardhat-deploy": "^0.7.0-beta.38", + "husky": "^5.1.3", "prettier": "^2.1.2", "prettier-plugin-solidity": "^1.0.0-alpha.60", "solhint": "^3.3.2", diff --git a/src/deploy/deploy_accessors.ts b/src/deploy/deploy_accessors.ts new file mode 100644 index 000000000..60a79da59 --- /dev/null +++ b/src/deploy/deploy_accessors.ts @@ -0,0 +1,20 @@ +import { DeployFunction } from "hardhat-deploy/types"; +import { HardhatRuntimeEnvironment } from "hardhat/types"; + +const deploy: DeployFunction = async function ( + hre: HardhatRuntimeEnvironment, +) { + const { deployments, getNamedAccounts } = hre; + const { deployer } = await getNamedAccounts(); + const { deploy } = deployments; + + await deploy("SimulateTxAccessor", { + from: deployer, + args: [], + log: true, + deterministicDeployment: true, + }); +}; + +deploy.tags = ['accessors'] +export default deploy; diff --git a/test/accessors/SimulateTxAccessor.spec.ts b/test/accessors/SimulateTxAccessor.spec.ts new file mode 100644 index 000000000..f5fecff67 --- /dev/null +++ b/test/accessors/SimulateTxAccessor.spec.ts @@ -0,0 +1,103 @@ +import { expect } from "chai"; +import hre, { deployments, waffle } from "hardhat"; +import "@nomiclabs/hardhat-ethers"; +import { deployContract, getSimulateTxAccessor, getSafeWithOwners } from "../utils/setup"; +import { buildContractCall } from "../utils/execution"; +import { parseEther } from "ethers/lib/utils"; + +describe("SimulateTxAccessor", async () => { + + const [user1, user2] = waffle.provider.getWallets(); + + const setupTests = deployments.createFixture(async ({ deployments }) => { + await deployments.fixture(); + const accessor = await getSimulateTxAccessor() + const source = ` + contract Test { + function sendAndReturnBalance(address payable target, uint256 amount) public returns (uint256) { + (bool success,) = target.call{ value: amount }(""); + require(success, "Transfer failed"); + return target.balance; + } + }` + const interactor = await deployContract(user1, source); + return { + safe: await getSafeWithOwners([user1.address]), + accessor, + interactor + } + }) + + describe("estimate", async () => { + + it('should enforce delegatecall', async () => { + const { accessor } = await setupTests() + const source = ` + contract Test { + function killme() public { + selfdestruct(payable(msg.sender)); + } + }` + const killLib = await deployContract(user1, source); + const tx = buildContractCall(killLib, "killme", [], 0) + + let code = await hre.ethers.provider.getCode(accessor.address) + await expect( + accessor.simulate(tx.to, tx.value, tx.data, tx.operation) + ).to.be.revertedWith("SimulateTxAccessor should only be called via delegatecall") + + expect(await hre.ethers.provider.getCode(accessor.address)).to.be.eq(code) + }) + + it('simulate call', async () => { + const { safe, accessor } = await setupTests() + const tx = buildContractCall(safe, "getOwners", [], 0) + const simulationData = accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.data, tx.operation]) + const acccessibleData = await safe.callStatic.simulateDelegatecall(accessor.address, simulationData) + const simulation = accessor.interface.decodeFunctionResult("simulate", acccessibleData) + expect( + safe.interface.decodeFunctionResult("getOwners", simulation.returnData)[0] + ).to.be.deep.eq([user1.address]) + expect( + simulation.success + ).to.be.true + expect( + simulation.estimate.toNumber() + ).to.be.lte(10000) + }) + + it('simulate delegatecall', async () => { + const { safe, accessor, interactor } = await setupTests() + await user1.sendTransaction({to: safe.address, value: parseEther("1")}) + const userBalance = await hre.ethers.provider.getBalance(user2.address) + const tx = buildContractCall(interactor, "sendAndReturnBalance", [user2.address, parseEther("1")], 0, true) + const simulationData = accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.data, tx.operation]) + const acccessibleData = await safe.callStatic.simulateDelegatecall(accessor.address, simulationData) + const simulation = accessor.interface.decodeFunctionResult("simulate", acccessibleData) + expect( + interactor.interface.decodeFunctionResult("sendAndReturnBalance", simulation.returnData)[0] + ).to.be.deep.eq(userBalance.add(parseEther("1"))) + expect( + simulation.success + ).to.be.true + expect( + simulation.estimate.toNumber() + ).to.be.lte(15000) + }) + + it('simulate revert', async () => { + const { safe, accessor, interactor } = await setupTests() + const tx = buildContractCall(interactor, "sendAndReturnBalance", [user2.address, parseEther("1")], 0, true) + const simulationData = accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.data, tx.operation]) + const acccessibleData = await safe.callStatic.simulateDelegatecall(accessor.address, simulationData) + const simulation = accessor.interface.decodeFunctionResult("simulate", acccessibleData) + expect(simulation.returnData).to.be.deep.eq("0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000f5472616e73666572206661696c65640000000000000000000000000000000000") + expect( + simulation.success + ).to.be.false + expect( + simulation.estimate.toNumber() + ).to.be.lte(20000) + }) + }) +}) \ No newline at end of file diff --git a/test/core/GnosisSafe.Estimation.spec.ts b/test/core/GnosisSafe.Estimation.spec.ts index dd2d29863..93bf461cc 100644 --- a/test/core/GnosisSafe.Estimation.spec.ts +++ b/test/core/GnosisSafe.Estimation.spec.ts @@ -33,7 +33,7 @@ describe("GnosisSafe", async () => { } }) - describe.only("requiredTxGas", async () => { + describe("requiredTxGas", async () => { it('should revert without reason if tx fails', async () => { const { safe, reverter } = await setupTests() diff --git a/test/core/GnosisSafe.StorageAccessible.spec.ts b/test/core/GnosisSafe.StorageAccessible.spec.ts new file mode 100644 index 000000000..b0a4157ed --- /dev/null +++ b/test/core/GnosisSafe.StorageAccessible.spec.ts @@ -0,0 +1,143 @@ +import { expect } from "chai"; +import hre, { deployments, waffle } from "hardhat"; +import "@nomiclabs/hardhat-ethers"; +import { deployContract, getSafeSingleton, getDefaultCallbackHandler, getSafeWithOwners } from "../utils/setup"; +import { BigNumber, utils } from "ethers"; + +describe("StorageAccessible", async () => { + + const [user1, user2] = waffle.provider.getWallets(); + + const setupTests = deployments.createFixture(async ({ deployments }) => { + await deployments.fixture(); + const handler = await getDefaultCallbackHandler() + const source = ` + contract Test { + function killme() public { + selfdestruct(payable(msg.sender)); + } + + function expose() public returns (address handler) { + bytes32 slot = 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5; + assembly { + handler := sload(slot) + } + } + + function estimate(address to, bytes memory data) public returns (uint256) { + uint256 startGas = gasleft(); + (bool success,) = to.call{ gas: gasleft() }(data); + require(success, "Transaction failed"); + return startGas - gasleft(); + } + + address singleton; + uint256 public value = 0; + function updateAndGet() public returns (uint256) { + value++; + return value; + } + + function trever() public returns (address handler) { + revert("Why are you doing this?"); + } + }` + const killLib = await deployContract(user1, source); + return { + safe: await getSafeWithOwners([user1.address, user2.address], 1, handler.address), + killLib, + handler + } + }) + + describe("getStorageAt", async () => { + + it('can read singleton', async () => { + await setupTests() + const singleton = await getSafeSingleton() + expect( + await singleton.getStorageAt(3, 2) + ).to.be.eq(utils.solidityPack(['uint256', 'uint256'], [0, 1])) + }) + + it('can read instantiated Safe', async () => { + const { safe } = await setupTests() + const singleton = await getSafeSingleton() + // Read singleton address, empty slots for module and owner linked lists, owner count and threshold + expect( + await safe.getStorageAt(0, 5) + ).to.be.eq(utils.solidityPack(['uint256', 'uint256', 'uint256', 'uint256', 'uint256'], [singleton.address, 0, 0, 2, 1])) + }) + }) + + describe("simulateDelegatecall", async () => { + + it('should revert changes', async () => { + const { safe, killLib } = await setupTests() + const code = await hre.ethers.provider.getCode(safe.address) + expect( + await safe.callStatic.simulateDelegatecall(killLib.address, killLib.interface.encodeFunctionData("killme")) + ).to.be.eq("0x") + expect( + await hre.ethers.provider.getCode(safe.address) + ).to.be.eq(code) + }) + + it('should return result', async () => { + const { safe, killLib, handler } = await setupTests() + expect( + await safe.callStatic.simulateDelegatecall(killLib.address, killLib.interface.encodeFunctionData("expose")) + ).to.be.eq("0x000000000000000000000000" + handler.address.slice(2).toLowerCase()) + }) + + it('should propagate revert message', async () => { + const { safe, killLib } = await setupTests() + await expect( + safe.callStatic.simulateDelegatecall(killLib.address, killLib.interface.encodeFunctionData("trever")) + ).to.revertedWith("Why are you doing this?") + }) + + it('should simulate transaction', async () => { + const { safe, killLib } = await setupTests() + const estimate = await safe.callStatic.simulateDelegatecall( + killLib.address, + killLib.interface.encodeFunctionData("estimate", [safe.address, "0x"]) + ) + expect(BigNumber.from(estimate).toNumber()).to.be.lte(5000) + }) + + it('should return modified state', async () => { + const { safe, killLib } = await setupTests() + const value = await safe.callStatic.simulateDelegatecall( + killLib.address, + killLib.interface.encodeFunctionData("updateAndGet", []) + ) + expect(BigNumber.from(value).toNumber()).to.be.eq(1) + expect((await killLib.value()).toNumber()).to.be.eq(0) + }) + }) + + describe("simulateDelegatecallInternal", async () => { + + it('should revert changes', async () => { + const { safe, killLib } = await setupTests() + await expect( + safe.callStatic.simulateDelegatecallInternal(killLib.address, killLib.interface.encodeFunctionData("killme")) + ).to.be.reverted + }) + + it('should revert the revert with message', async () => { + const { safe, killLib } = await setupTests() + await expect( + safe.callStatic.simulateDelegatecallInternal(killLib.address, killLib.interface.encodeFunctionData("trever")) + ).to.revertedWith("Why are you doing this?") + }) + + it('should return estimate in revert', async () => { + const { safe, killLib } = await setupTests() + await expect( + safe.callStatic.simulateDelegatecallInternal(killLib.address, killLib.interface.encodeFunctionData("estimate", [safe.address, "0x"])) + ).to.be.reverted + }) + }) +}) \ No newline at end of file diff --git a/test/utils/setup.ts b/test/utils/setup.ts index a174f0882..54b73c3a0 100644 --- a/test/utils/setup.ts +++ b/test/utils/setup.ts @@ -23,6 +23,12 @@ export const getFactory = async () => { return Factory.attach(FactoryDeployment.address); } +export const getSimulateTxAccessor = async () => { + const SimulateTxAccessorDeployment = await deployments.get("SimulateTxAccessor"); + const SimulateTxAccessor = await hre.ethers.getContractFactory("SimulateTxAccessor"); + return SimulateTxAccessor.attach(SimulateTxAccessorDeployment.address); +} + export const getMultiSend = async () => { const MultiSendDeployment = await deployments.get("MultiSend"); const MultiSend = await hre.ethers.getContractFactory("MultiSend"); @@ -49,9 +55,9 @@ export const getSafeTemplate = async () => { return Safe.attach(template); } -export const getSafeWithOwners = async (owners: string[], threhsold?: number, fallbackHandler?: string) => { +export const getSafeWithOwners = async (owners: string[], threshold?: number, fallbackHandler?: string) => { const template = await getSafeTemplate() - await template.setup(owners, threhsold || owners.length, AddressZero, "0x", fallbackHandler || AddressZero, AddressZero, 0, AddressZero) + await template.setup(owners, threshold || owners.length, AddressZero, "0x", fallbackHandler || AddressZero, AddressZero, 0, AddressZero) return template } diff --git a/yarn.lock b/yarn.lock index 526bb7357..038ad2a7b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4859,6 +4859,11 @@ https-proxy-agent@^5.0.0: agent-base "6" debug "4" +husky@^5.1.3: + version "5.1.3" + resolved "https://registry.yarnpkg.com/husky/-/husky-5.1.3.tgz#1a0645a4fe3ffc006c4d0d8bd0bcb4c98787cc9d" + integrity sha512-fbNJ+Gz5wx2LIBtMweJNY1D7Uc8p1XERi5KNRMccwfQA+rXlxWNSdUxswo0gT8XqxywTIw7Ywm/F4v/O35RdMg== + iconv-lite@0.4.24, iconv-lite@^0.4.24: version "0.4.24" resolved "https://registry.yarnpkg.com/iconv-lite/-/iconv-lite-0.4.24.tgz#2022b4b25fbddc21d2f524974a474aafe733908b"