diff --git a/contracts/Safe.sol b/contracts/Safe.sol index 77a124098..f0541ab05 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -192,7 +192,7 @@ contract Safe is // 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, "GS010"); + if (gasleft() < ((safeTxGas * 64) / 63).max(safeTxGas + 2500) + 500) revertWithError("GS010"); // Use scope here to limit variable lifetime and prevent `stack too deep` errors { uint256 gasUsed = gasleft(); @@ -202,7 +202,7 @@ contract Safe is gasUsed = gasUsed.sub(gasleft()); // If no safeTxGas and no gasPrice was set (e.g. both are 0), then the internal tx is required to be successful // This makes it possible to use `estimateGas` without issues, as it searches for the minimum gas where the tx doesn't revert - require(success || safeTxGas != 0 || gasPrice != 0, "GS013"); + if (!success && safeTxGas == 0 && gasPrice == 0) revertWithError("GS013"); // We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls uint256 payment = 0; if (gasPrice > 0) { @@ -239,10 +239,10 @@ contract Safe is // For native tokens, we will only adjust the gas price to not be higher than the actually used gas price payment = gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice); (bool refundSuccess, ) = receiver.call{value: payment}(""); - require(refundSuccess, "GS011"); + if (!refundSuccess) revertWithError("GS011"); } else { payment = gasUsed.add(baseGas).mul(gasPrice); - require(transferToken(gasToken, receiver, payment), "GS012"); + if (!transferToken(gasToken, receiver, payment)) revertWithError("GS012"); } } @@ -257,7 +257,7 @@ contract Safe is */ function checkContractSignature(address owner, bytes32 dataHash, bytes memory signatures, uint256 offset) internal view { // Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes) - require(offset.add(32) <= signatures.length, "GS022"); + if (offset.add(32) > signatures.length) revertWithError("GS022"); // Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length uint256 contractSignatureLen; @@ -267,7 +267,7 @@ contract Safe is contractSignatureLen := mload(add(add(signatures, offset), 0x20)) } /* solhint-enable no-inline-assembly */ - require(offset.add(32).add(contractSignatureLen) <= signatures.length, "GS023"); + if (offset.add(32).add(contractSignatureLen) > signatures.length) revertWithError("GS023"); // Check signature bytes memory contractSignature; @@ -279,7 +279,7 @@ contract Safe is } /* solhint-enable no-inline-assembly */ - require(ISignatureValidator(owner).isValidSignature(dataHash, contractSignature) == EIP1271_MAGIC_VALUE, "GS024"); + if (ISignatureValidator(owner).isValidSignature(dataHash, contractSignature) != EIP1271_MAGIC_VALUE) revertWithError("GS024"); } /** @@ -303,7 +303,7 @@ contract Safe is // Load threshold to avoid multiple storage loads uint256 _threshold = threshold; // Check that a threshold is set - require(_threshold > 0, "GS001"); + if (_threshold == 0) revertWithError("GS001"); checkNSignatures(msg.sender, dataHash, signatures, _threshold); } @@ -320,11 +320,11 @@ contract Safe is */ function checkNSignatures(address executor, bytes32 dataHash, bytes memory signatures, uint256 requiredSignatures) public view { // Check that the provided signature data is not too short - require(signatures.length >= requiredSignatures.mul(65), "GS020"); + if (signatures.length < requiredSignatures.mul(65)) revertWithError("GS020"); // There cannot be an owner with address 0. address lastOwner = address(0); address currentOwner; - uint8 v; + uint256 v; // Implicit conversion from uint8 to uint256 will be done for v received from signatureSplit(...). bytes32 r; bytes32 s; uint256 i; @@ -338,7 +338,7 @@ contract Safe is // 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) >= requiredSignatures.mul(65), "GS021"); + if (uint256(s) < requiredSignatures.mul(65)) revertWithError("GS021"); // The contract signature check is extracted to a separate function for better compatibility with formal verification // A quote from the Certora team: @@ -350,17 +350,18 @@ contract Safe is // 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(executor == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025"); + if (executor != currentOwner && approvedHashes[currentOwner][dataHash] == 0) revertWithError("GS025"); } else if (v > 30) { // If v > 30 then default va (27,28) has been adjusted for eth_sign flow // 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); + currentOwner = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), uint8(v - 4), r, s); } else { // Default is the ecrecover flow with the provided data hash // Use ecrecover with the messageHash for EOA signatures - currentOwner = ecrecover(dataHash, v, r, s); + currentOwner = ecrecover(dataHash, uint8(v), r, s); } - require(currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS, "GS026"); + if (currentOwner <= lastOwner || owners[currentOwner] == address(0) || currentOwner == SENTINEL_OWNERS) + revertWithError("GS026"); lastOwner = currentOwner; } } @@ -372,7 +373,7 @@ contract Safe is * @param hashToApprove The hash to mark as approved for signatures that are verified by this contract. */ function approveHash(bytes32 hashToApprove) external { - require(owners[msg.sender] != address(0), "GS030"); + if (owners[msg.sender] == address(0)) revertWithError("GS030"); approvedHashes[msg.sender][hashToApprove] = 1; emit ApproveHash(hashToApprove, msg.sender); } diff --git a/contracts/base/FallbackManager.sol b/contracts/base/FallbackManager.sol index 67d59d557..4203974f1 100644 --- a/contracts/base/FallbackManager.sol +++ b/contracts/base/FallbackManager.sol @@ -31,13 +31,12 @@ abstract contract FallbackManager is SelfAuthorized { where the first 3 bytes of the previous calldata + the first byte of the address make up a valid function signature. The subsequent call would result in unsanctioned access to Safe's internal protected methods. For some reason, solidity matches the first 4 bytes of the calldata to a function signature, regardless if more data follow these 4 bytes. */ - require(handler != address(this), "GS400"); + if (handler == address(this)) revertWithError("GS400"); - bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT; /* solhint-disable no-inline-assembly */ /// @solidity memory-safe-assembly assembly { - sstore(slot, handler) + sstore(FALLBACK_HANDLER_STORAGE_SLOT, handler) } /* solhint-enable no-inline-assembly */ } @@ -61,7 +60,6 @@ abstract contract FallbackManager is SelfAuthorized { // and having the original caller address may enable additional verification scenarios. // solhint-disable-next-line payable-fallback,no-complex-fallback fallback() external { - bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT; /* solhint-disable no-inline-assembly */ /// @solidity memory-safe-assembly assembly { @@ -69,7 +67,9 @@ abstract contract FallbackManager is SelfAuthorized { // memory is used, therefore we need to guarantee memory safety (keeping the free memory point 0x40 slot intact, // not going beyond the scratch space, etc) // Solidity docs: https://docs.soliditylang.org/en/latest/assembly.html#memory-safety - let handler := sload(slot) + + let handler := sload(FALLBACK_HANDLER_STORAGE_SLOT) + if iszero(handler) { return(0, 0) } diff --git a/contracts/base/GuardManager.sol b/contracts/base/GuardManager.sol index 5b0c91549..d71799f64 100644 --- a/contracts/base/GuardManager.sol +++ b/contracts/base/GuardManager.sol @@ -86,14 +86,11 @@ abstract contract GuardManager is SelfAuthorized { * @param guard The address of the guard to be used or the 0 address to disable the guard */ function setGuard(address guard) external authorized { - if (guard != address(0)) { - require(Guard(guard).supportsInterface(type(Guard).interfaceId), "GS300"); - } - bytes32 slot = GUARD_STORAGE_SLOT; + if (guard != address(0) && !Guard(guard).supportsInterface(type(Guard).interfaceId)) revertWithError("GS300"); /* solhint-disable no-inline-assembly */ /// @solidity memory-safe-assembly assembly { - sstore(slot, guard) + sstore(GUARD_STORAGE_SLOT, guard) } /* solhint-enable no-inline-assembly */ emit ChangedGuard(guard); @@ -107,11 +104,10 @@ abstract contract GuardManager is SelfAuthorized { * @return guard The address of the guard */ function getGuard() internal view returns (address guard) { - bytes32 slot = GUARD_STORAGE_SLOT; /* solhint-disable no-inline-assembly */ /// @solidity memory-safe-assembly assembly { - guard := sload(slot) + guard := sload(GUARD_STORAGE_SLOT) } /* solhint-enable no-inline-assembly */ } diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 7e0c49e2d..45bc0bbb0 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -31,12 +31,12 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager { * @param data Optional data of call to execute. */ function setupModules(address to, bytes memory data) internal { - require(modules[SENTINEL_MODULES] == address(0), "GS100"); + if (modules[SENTINEL_MODULES] != address(0)) revertWithError("GS100"); modules[SENTINEL_MODULES] = SENTINEL_MODULES; if (to != address(0)) { - require(isContract(to), "GS002"); + if (!isContract(to)) revertWithError("GS002"); // Setup has to complete successfully or transaction fails. - require(execute(to, 0, data, Enum.Operation.DelegateCall, type(uint256).max), "GS000"); + if (!execute(to, 0, data, Enum.Operation.DelegateCall, type(uint256).max)) revertWithError("GS000"); } } @@ -87,9 +87,9 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager { */ function enableModule(address module) public authorized { // Module address cannot be null or sentinel. - require(module != address(0) && module != SENTINEL_MODULES, "GS101"); + if (module == address(0) || module == SENTINEL_MODULES) revertWithError("GS101"); // Module cannot be added twice. - require(modules[module] == address(0), "GS102"); + if (modules[module] != address(0)) revertWithError("GS102"); modules[module] = modules[SENTINEL_MODULES]; modules[SENTINEL_MODULES] = module; emit EnabledModule(module); @@ -103,8 +103,8 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager { */ function disableModule(address prevModule, address module) public authorized { // Validate module address and check that it corresponds to module index. - require(module != address(0) && module != SENTINEL_MODULES, "GS101"); - require(modules[prevModule] == module, "GS103"); + if (module == address(0) || module == SENTINEL_MODULES) revertWithError("GS101"); + if (modules[prevModule] != module) revertWithError("GS103"); modules[prevModule] = modules[module]; modules[module] = address(0); emit DisabledModule(module); @@ -182,8 +182,8 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager { * @return next Start of the next page. */ function getModulesPaginated(address start, uint256 pageSize) external view returns (address[] memory array, address next) { - require(start == SENTINEL_MODULES || isModuleEnabled(start), "GS105"); - require(pageSize > 0, "GS106"); + if (start != SENTINEL_MODULES && !isModuleEnabled(start)) revertWithError("GS105"); + if (pageSize == 0) revertWithError("GS106"); // Init array with max page size array = new address[](pageSize); diff --git a/contracts/base/OwnerManager.sol b/contracts/base/OwnerManager.sol index 1d121a59c..9ffb5a6b9 100644 --- a/contracts/base/OwnerManager.sol +++ b/contracts/base/OwnerManager.sol @@ -28,19 +28,20 @@ abstract contract OwnerManager is SelfAuthorized { function setupOwners(address[] memory _owners, uint256 _threshold) internal { // Threshold can only be 0 at initialization. // Check ensures that setup function can only be called once. - require(threshold == 0, "GS200"); + if (threshold > 0) revertWithError("GS200"); // Validate that threshold is smaller than number of added owners. - require(_threshold <= _owners.length, "GS201"); + if (_threshold > _owners.length) revertWithError("GS201"); // There has to be at least one Safe owner. - require(_threshold >= 1, "GS202"); + if (_threshold == 0) revertWithError("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, "GS203"); + if (owner == address(0) || owner == SENTINEL_OWNERS || owner == address(this) || currentOwner == owner) + revertWithError("GS203"); // No duplicate owners allowed. - require(owners[owner] == address(0), "GS204"); + if (owners[owner] != address(0)) revertWithError("GS204"); owners[currentOwner] = owner; currentOwner = owner; } @@ -57,9 +58,9 @@ abstract contract OwnerManager is SelfAuthorized { */ function addOwnerWithThreshold(address owner, uint256 _threshold) public authorized { // Owner address cannot be null, the sentinel or the Safe itself. - require(owner != address(0) && owner != SENTINEL_OWNERS && owner != address(this), "GS203"); + if (owner == address(0) || owner == SENTINEL_OWNERS || owner == address(this)) revertWithError("GS203"); // No duplicate owners allowed. - require(owners[owner] == address(0), "GS204"); + if (owners[owner] != address(0)) revertWithError("GS204"); owners[owner] = owners[SENTINEL_OWNERS]; owners[SENTINEL_OWNERS] = owner; ownerCount++; @@ -77,10 +78,10 @@ abstract contract OwnerManager is SelfAuthorized { */ function removeOwner(address prevOwner, address owner, uint256 _threshold) public authorized { // Only allow to remove an owner, if threshold can still be reached. - require(ownerCount - 1 >= _threshold, "GS201"); + if (ownerCount - 1 < _threshold) revertWithError("GS201"); // Validate owner address and check that it corresponds to owner index. - require(owner != address(0) && owner != SENTINEL_OWNERS, "GS203"); - require(owners[prevOwner] == owner, "GS205"); + if (owner == address(0) || owner == SENTINEL_OWNERS) revertWithError("GS203"); + if (owners[prevOwner] != owner) revertWithError("GS205"); owners[prevOwner] = owners[owner]; owners[owner] = address(0); ownerCount--; @@ -98,12 +99,12 @@ abstract contract OwnerManager is SelfAuthorized { */ function swapOwner(address prevOwner, address oldOwner, address newOwner) public authorized { // Owner address cannot be null, the sentinel or the Safe itself. - require(newOwner != address(0) && newOwner != SENTINEL_OWNERS && newOwner != address(this), "GS203"); + if (newOwner == address(0) || newOwner == SENTINEL_OWNERS || newOwner == address(this)) revertWithError("GS203"); // No duplicate owners allowed. - require(owners[newOwner] == address(0), "GS204"); + if (owners[newOwner] != address(0)) revertWithError("GS204"); // Validate oldOwner address and check that it corresponds to owner index. - require(oldOwner != address(0) && oldOwner != SENTINEL_OWNERS, "GS203"); - require(owners[prevOwner] == oldOwner, "GS205"); + if (oldOwner == address(0) || oldOwner == SENTINEL_OWNERS) revertWithError("GS203"); + if (owners[prevOwner] != oldOwner) revertWithError("GS205"); owners[newOwner] = owners[oldOwner]; owners[prevOwner] = newOwner; owners[oldOwner] = address(0); @@ -118,9 +119,9 @@ abstract contract OwnerManager is SelfAuthorized { */ function changeThreshold(uint256 _threshold) public authorized { // Validate that threshold is smaller than number of owners. - require(_threshold <= ownerCount, "GS201"); + if (_threshold > ownerCount) revertWithError("GS201"); // There has to be at least one Safe owner. - require(_threshold >= 1, "GS202"); + if (_threshold == 0) revertWithError("GS202"); threshold = _threshold; emit ChangedThreshold(threshold); } @@ -138,7 +139,7 @@ abstract contract OwnerManager is SelfAuthorized { * @return Boolean if owner is an owner of the Safe. */ function isOwner(address owner) public view returns (bool) { - return owner != SENTINEL_OWNERS && owners[owner] != address(0); + return !(owner == SENTINEL_OWNERS || owners[owner] == address(0)); } /** diff --git a/contracts/common/SelfAuthorized.sol b/contracts/common/SelfAuthorized.sol index 4e60bb9d6..966098fa2 100644 --- a/contracts/common/SelfAuthorized.sol +++ b/contracts/common/SelfAuthorized.sol @@ -1,13 +1,15 @@ // SPDX-License-Identifier: LGPL-3.0-only pragma solidity >=0.7.0 <0.9.0; +import {ErrorMessage} from "../libraries/ErrorMessage.sol"; + /** * @title SelfAuthorized - Authorizes current contract to perform actions to itself. * @author Richard Meissner - @rmeissner */ -abstract contract SelfAuthorized { +abstract contract SelfAuthorized is ErrorMessage { function requireSelfCall() private view { - require(msg.sender == address(this), "GS031"); + if (msg.sender != address(this)) revertWithError("GS031"); } modifier authorized() { diff --git a/contracts/libraries/ErrorMessage.sol b/contracts/libraries/ErrorMessage.sol new file mode 100644 index 000000000..a18d8846f --- /dev/null +++ b/contracts/libraries/ErrorMessage.sol @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: LGPL-3.0-only + +pragma solidity >=0.7.0 <0.9.0; + +/** + * @title Error Message - Contract which uses assembly to revert with a custom error message. + * @author Shebin John - @remedcu + * @notice The aim is to save gas using assembly to revert with custom error message. + */ +abstract contract ErrorMessage { + /** + * @notice Function which uses assembly to revert with the passed error message. + * @param error The error string to revert with. + * @dev Currently it is expected that the `error` string is at max 5 bytes of length. Ex: "GSXXX" + */ + function revertWithError(bytes5 error) internal pure { + /* solhint-disable no-inline-assembly */ + /// @solidity memory-safe-assembly + assembly { + let ptr := mload(0x40) + mstore(ptr, 0x08c379a000000000000000000000000000000000000000000000000000000000) // Selector for method "Error(string)" + mstore(add(ptr, 0x04), 0x20) // String offset + mstore(add(ptr, 0x24), 0x05) // Revert reason length (5 bytes for bytes5) + mstore(add(ptr, 0x44), error) // Revert reason + revert(ptr, 0x64) // Revert data length is 4 bytes for selector + offset + error length + error. + } + /* solhint-enable no-inline-assembly */ + } +}