Gas optimizations (#277)

Author: rmeissner

.github/workflows/ci.yml

@@ -41,7 +41,4 @@ jobs:
         with:
           path: "**/node_modules"
           key: ${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }}
-      - run: yarn
-      - run: yarn build
-      - run: yarn hardhat codesize --contractname GnosisSafe
-      - run: yarn benchmark
+      - run: (yarn && yarn build && yarn hardhat codesize --contractname GnosisSafe && yarn benchmark) || echo "Benchmark failed"

README.md

@@ -60,6 +60,7 @@ yarn hardhat --network <network> etherscan-verify
 Documentation
 -------------
 - [Safe developer portal](http://docs.gnosis.io/safe)
+- [Error codes](docs/error_codes.md)
 - [Coding guidelines](docs/guidelines.md)
 
 Audits/ Formal Verification

contracts/GnosisSafe.sol

@@ -21,7 +21,6 @@ contract GnosisSafe
 
     using GnosisSafeMath for uint256;
 
-    string public constant NAME = "Gnosis Safe";
     string public constant VERSION = "1.2.0";
 
     // keccak256(
@@ -117,7 +116,7 @@ contract GnosisSafe
     function execTransaction(
         address to,
         uint256 value,
-        bytes memory data,
+        bytes calldata data,
         Enum.Operation operation,
         uint256 safeTxGas,
         uint256 baseGas,
@@ -155,7 +154,7 @@ contract GnosisSafe
         }
         // 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
-        require(gasleft() >= (safeTxGas * 64 / 63).max(safeTxGas + 2500) + 500, "Not enough gas to execute safe transaction");
+        require(gasleft() >= (safeTxGas * 64 / 63).max(safeTxGas + 2500) + 500, "GS010");
         // Use scope here to limit variable lifetime and prevent `stack too deep` errors
         {
             uint256 gasUsed = gasleft();
@@ -189,10 +188,10 @@ contract GnosisSafe
             // For ETH we will only adjust the gas price to not be higher than the actual used gas price
             payment = gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice);
             // solium-disable-next-line security/no-send
-            require(receiver.send(payment), "Could not pay gas costs with ether");
+            require(receiver.send(payment), "GS011");
         } else {
             payment = gasUsed.add(baseGas).mul(gasPrice);
-            require(transferToken(gasToken, receiver, payment), "Could not pay gas costs with token");
+            require(transferToken(gasToken, receiver, payment), "GS012");
         }
     }
 
@@ -209,9 +208,9 @@ contract GnosisSafe
         // Load threshold to avoid multiple storage loads
         uint256 _threshold = threshold;
         // Check that a threshold is set
-        require(_threshold > 0, "Threshold needs to be defined!");
+        require(_threshold > 0, "GS001");
         // Check that the provided signature data is not too short
-        require(signatures.length >= _threshold.mul(65), "Signatures data too short");
+        require(signatures.length >= _threshold.mul(65), "GS020");
         // There cannot be an owner with address 0.
         address lastOwner = address(0);
         address currentOwner;
@@ -229,18 +228,18 @@ contract GnosisSafe
                 // Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes
                 // This check is not completely accurate, since it is possible that more signatures than the threshold are send.
                 // Here we only check that the pointer is not pointing inside the part that is being processed
-                require(uint256(s) >= _threshold.mul(65), "Invalid contract signature location: inside static part");
+                require(uint256(s) >= _threshold.mul(65), "GS021");
 
                 // Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes)
-                require(uint256(s).add(32) <= signatures.length, "Invalid contract signature location: length not present");
+                require(uint256(s).add(32) <= signatures.length, "GS022");
 
                 // Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length
                 uint256 contractSignatureLen;
                 // solium-disable-next-line security/no-inline-assembly
                 assembly {
                     contractSignatureLen := mload(add(add(signatures, s), 0x20))
                 }
-                require(uint256(s).add(32).add(contractSignatureLen) <= signatures.length, "Invalid contract signature location: data not complete");
+                require(uint256(s).add(32).add(contractSignatureLen) <= signatures.length, "GS023");
 
                 // Check signature
                 bytes memory contractSignature;
@@ -249,13 +248,13 @@ contract GnosisSafe
                     // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
                     contractSignature := add(add(signatures, s), 0x20)
                 }
-                require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "Invalid contract signature provided");
+                require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "GS024");
             // If v is 1 then it is an approved hash
             } else if (v == 1) {
                 // When handling approved hashes the address of the approver is encoded into r
                 currentOwner = address(uint160(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");
+                require(msg.sender == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025");
             } 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);
@@ -265,7 +264,7 @@ contract GnosisSafe
             }
             require (
                 currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS,
-                "Invalid owner provided"
+                "GS026"
             );
             lastOwner = currentOwner;
         }
@@ -300,7 +299,7 @@ contract GnosisSafe
     function approveHash(bytes32 hashToApprove)
         external
     {
-        require(owners[msg.sender] != address(0), "Only owners can approve a hash");
+        require(owners[msg.sender] != address(0), "GS030");
         approvedHashes[msg.sender][hashToApprove] = 1;
         emit ApproveHash(hashToApprove, msg.sender);
     }
@@ -366,7 +365,7 @@ contract GnosisSafe
     function encodeTransactionData(
         address to,
         uint256 value,
-        bytes memory data,
+        bytes calldata data,
         Enum.Operation operation,
         uint256 safeTxGas,
         uint256 baseGas,
@@ -400,7 +399,7 @@ contract GnosisSafe
     function getTransactionHash(
         address to,
         uint256 value,
-        bytes memory data,
+        bytes calldata data,
         Enum.Operation operation,
         uint256 safeTxGas,
         uint256 baseGas,

contracts/accessors/SimulateTxAccessor.sol

@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity >=0.7.0 <0.9.0;
 
 import "../base/Executor.sol";
@@ -9,7 +10,7 @@ contract SimulateTxAccessor is Executor {
     bytes32 constant private GUARD_VALUE = keccak256("simulate_tx_accessor.guard.bytes32");
     bytes32 guard;
 
-    constructor() public {
+    constructor() {
         guard = GUARD_VALUE;
     }
 

contracts/base/ModuleManager.sol

@@ -22,11 +22,11 @@ contract ModuleManager is SelfAuthorized, Executor {
     function setupModules(address to, bytes memory data)
         internal
     {
-        require(modules[SENTINEL_MODULES] == address(0), "Modules have already been initialized");
+        require(modules[SENTINEL_MODULES] == address(0), "GS100");
         modules[SENTINEL_MODULES] = SENTINEL_MODULES;
         if (to != address(0))
             // Setup has to complete successfully or transaction fails.
-            require(execute(to, 0, data, Enum.Operation.DelegateCall, gasleft()), "Could not finish initialization");
+            require(execute(to, 0, data, Enum.Operation.DelegateCall, gasleft()), "GS000");
     }
 
     /// @dev Allows to add a module to the whitelist.
@@ -38,9 +38,9 @@ contract ModuleManager is SelfAuthorized, Executor {
         authorized
     {
         // Module address cannot be null or sentinel.
-        require(module != address(0) && module != SENTINEL_MODULES, "Invalid module address provided");
+        require(module != address(0) && module != SENTINEL_MODULES, "GS101");
         // Module cannot be added twice.
-        require(modules[module] == address(0), "Module has already been added");
+        require(modules[module] == address(0), "GS102");
         modules[module] = modules[SENTINEL_MODULES];
         modules[SENTINEL_MODULES] = module;
         emit EnabledModule(module);
@@ -56,8 +56,8 @@ contract ModuleManager is SelfAuthorized, Executor {
         authorized
     {
         // Validate module address and check that it corresponds to module index.
-        require(module != address(0) && module != SENTINEL_MODULES, "Invalid module address provided");
-        require(modules[prevModule] == module, "Invalid prevModule, module pair provided");
+        require(module != address(0) && module != SENTINEL_MODULES, "GS101");
+        require(modules[prevModule] == module, "GS103");
         modules[prevModule] = modules[module];
         modules[module] = address(0);
         emit DisabledModule(module);
@@ -73,7 +73,7 @@ contract ModuleManager is SelfAuthorized, Executor {
         returns (bool success)
     {
         // Only whitelisted modules are allowed.
-        require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "Method can only be called from an enabled module");
+        require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "GS104");
         // Execute transaction without further confirmations.
         success = execute(to, value, data, operation, gasleft());
         if (success) emit ExecutionFromModuleSuccess(msg.sender);

contracts/base/OwnerManager.sol

@@ -25,19 +25,19 @@ contract OwnerManager is SelfAuthorized {
     {
         // Threshold can only be 0 at initialization.
         // Check ensures that setup function can only be called once.
-        require(threshold == 0, "Owners have already been setup");
+        require(threshold == 0, "GS200");
         // Validate that threshold is smaller than number of added owners.
-        require(_threshold <= _owners.length, "Threshold cannot exceed owner count");
+        require(_threshold <= _owners.length, "GS201");
         // There has to be at least one Safe owner.
-        require(_threshold >= 1, "Threshold needs to be greater than 0");
+        require(_threshold >= 1, "GS202");
         // Initializing Safe owners.
         address currentOwner = SENTINEL_OWNERS;
         for (uint256 i = 0; i < _owners.length; i++) {
             // Owner address cannot be null.
             address owner = _owners[i];
-            require(owner != address(0) && owner != SENTINEL_OWNERS && owner != address(this) && currentOwner != owner, "Invalid owner address provided");
+            require(owner != address(0) && owner != SENTINEL_OWNERS && owner != address(this) && currentOwner != owner, "GS203");
             // No duplicate owners allowed.
-            require(owners[owner] == address(0), "Duplicate owner address provided");
+            require(owners[owner] == address(0), "GS204");
             owners[currentOwner] = owner;
             currentOwner = owner;
         }
@@ -56,9 +56,9 @@ contract OwnerManager is SelfAuthorized {
         authorized
     {
         // Owner address cannot be null, the sentinel or the Safe itself.
-        require(owner != address(0) && owner != SENTINEL_OWNERS && owner != address(this), "Invalid owner address provided");
+        require(owner != address(0) && owner != SENTINEL_OWNERS && owner != address(this), "GS203");
         // No duplicate owners allowed.
-        require(owners[owner] == address(0), "Address is already an owner");
+        require(owners[owner] == address(0), "GS204");
         owners[owner] = owners[SENTINEL_OWNERS];
         owners[SENTINEL_OWNERS] = owner;
         ownerCount++;
@@ -79,10 +79,10 @@ contract OwnerManager is SelfAuthorized {
         authorized
     {
         // Only allow to remove an owner, if threshold can still be reached.
-        require(ownerCount - 1 >= _threshold, "New owner count needs to be larger than new threshold");
+        require(ownerCount - 1 >= _threshold, "GS201");
         // Validate owner address and check that it corresponds to owner index.
-        require(owner != address(0) && owner != SENTINEL_OWNERS, "Invalid owner address provided");
-        require(owners[prevOwner] == owner, "Invalid prevOwner, owner pair provided");
+        require(owner != address(0) && owner != SENTINEL_OWNERS, "GS203");
+        require(owners[prevOwner] == owner, "GS205");
         owners[prevOwner] = owners[owner];
         owners[owner] = address(0);
         ownerCount--;
@@ -103,12 +103,12 @@ contract OwnerManager is SelfAuthorized {
         authorized
     {
         // Owner address cannot be null, the sentinel or the Safe itself.
-        require(newOwner != address(0) && newOwner != SENTINEL_OWNERS && newOwner != address(this), "Invalid owner address provided");
+        require(newOwner != address(0) && newOwner != SENTINEL_OWNERS && newOwner != address(this), "GS203");
         // No duplicate owners allowed.
-        require(owners[newOwner] == address(0), "Address is already an owner");
+        require(owners[newOwner] == address(0), "GS204");
         // Validate oldOwner address and check that it corresponds to owner index.
-        require(oldOwner != address(0) && oldOwner != SENTINEL_OWNERS, "Invalid owner address provided");
-        require(owners[prevOwner] == oldOwner, "Invalid prevOwner, owner pair provided");
+        require(oldOwner != address(0) && oldOwner != SENTINEL_OWNERS, "GS203");
+        require(owners[prevOwner] == oldOwner, "GS205");
         owners[newOwner] = owners[oldOwner];
         owners[prevOwner] = newOwner;
         owners[oldOwner] = address(0);
@@ -125,9 +125,9 @@ contract OwnerManager is SelfAuthorized {
         authorized
     {
         // Validate that threshold is smaller than number of owners.
-        require(_threshold <= ownerCount, "Threshold cannot exceed owner count");
+        require(_threshold <= ownerCount, "GS201");
         // There has to be at least one Safe owner.
-        require(_threshold >= 1, "Threshold needs to be greater than 0");
+        require(_threshold >= 1, "GS202");
         threshold = _threshold;
         emit ChangedThreshold(threshold);
     }

contracts/common/SecuredTokenTransfer.sol

@@ -18,7 +18,8 @@ contract SecuredTokenTransfer {
         internal
         returns (bool transferred)
     {
-        bytes memory data = abi.encodeWithSignature("transfer(address,uint256)", receiver, amount);
+        // 0xa9059cbb - keccack("transfer(address,uint256)")
+        bytes memory data = abi.encodeWithSelector(0xa9059cbb, receiver, amount);
         // solium-disable-next-line security/no-inline-assembly
         assembly {
             let success := call(sub(gas(), 10000), token, 0, add(data, 0x20), mload(data), 0, 0)

contracts/common/SelfAuthorized.sol

@@ -6,7 +6,7 @@ pragma solidity >=0.7.0 <0.9.0;
 /// @author Richard Meissner - <richard@gnosis.pm>
 contract SelfAuthorized {
     function requireSelfCall() private view {
-        require(msg.sender == address(this), "Method can only be called from this contract");
+        require(msg.sender == address(this), "GS031");
     }
 
     modifier authorized() {

contracts/common/StorageAccessible.sol

@@ -4,10 +4,6 @@ pragma solidity >=0.7.0 <0.9.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
@@ -29,59 +25,24 @@ contract StorageAccessible {
         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(
+    function simulate(
         address targetContract,
-        bytes memory calldataPayload
-    ) public returns (bytes memory) {
+        bytes calldata calldataPayload
+    ) external 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 {
+        bytes memory responseWithStatus = abi.encodePacked(response, success);
         // solium-disable-next-line security/no-inline-assembly
         assembly {
-            mstore(buffer, length)
+            revert(add(responseWithStatus, 0x20), mload(responseWithStatus))
         }
     }
 }