Skip to content

Commit

Permalink
Merge pull request #726 from safe-global/contract-size
Browse files Browse the repository at this point in the history 
Reducing Safe Contract Code Size
  • Loading branch information
@remedcu
remedcu committed Jan 12, 2024
2 parents 1bc5036 + fba7acc commit ac0eda0
Show file tree
Hide file tree
Showing 7 changed files with 85 additions and 56 deletions.
33 changes: 17 additions & 16 deletions contracts/Safe.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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) {
Expand Down Expand Up @@ -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");
}
}

Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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");
}

/**
Expand All @@ -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);
}

Expand All @@ -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;
Expand All @@ -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:
Expand All @@ -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;
}
}
Expand All @@ -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);
}
Expand Down
10 changes: 5 additions & 5 deletions contracts/base/FallbackManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
}
Expand All @@ -61,15 +60,16 @@ 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 {
// When compiled with the optimizer, the compiler relies on a certain assumptions on how the
// 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)
}
Expand Down
10 changes: 3 additions & 7 deletions contracts/base/GuardManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 */
}
Expand Down
18 changes: 9 additions & 9 deletions contracts/base/ModuleManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand Down
35 changes: 18 additions & 17 deletions contracts/base/OwnerManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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++;
Expand All @@ -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--;
Expand All @@ -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);
Expand All @@ -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);
}
Expand All @@ -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));
}

/**
Expand Down
6 changes: 4 additions & 2 deletions contracts/common/SelfAuthorized.sol
View file
Original file line number Diff line number Diff line change
@@ -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() {
Expand Down

0 comments on commit ac0eda0

Please sign in to comment.