Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reducing Safe Contract Code Size #726

Merged
merged 13 commits into from
Jan 12, 2024
33 changes: 17 additions & 16 deletions contracts/Safe.sol
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");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such a surprise to me that changing require to if affects the code size...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now, its because of how the require was encoding the revert error.

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I've also noticed that keeping uintN for N < 256 tends to generate more code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learned from the best: #707 (comment)

} 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
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
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
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
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));
nlordell marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
6 changes: 4 additions & 2 deletions contracts/common/SelfAuthorized.sol
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