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

Adding ISafe interface for CompatibilityFallbackHandler #722

Merged
merged 24 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4b9eb4a
Adding ISafe and changing CompatibilityFallbackHandler
remedcu Dec 27, 2023
9efc0b1
Added codesize script in package.json
remedcu Dec 27, 2023
f50e204
IEnum added for interface inheritance
remedcu Dec 28, 2023
bf617f5
ISafe updated with IEnum and some more functions
remedcu Dec 28, 2023
4918270
Using ISafe from interfaces for contracts
remedcu Dec 28, 2023
2c4a9d0
Merge branch 'main' into ISafe
remedcu Jan 4, 2024
cc89e43
solhint warning rectified
remedcu Jan 4, 2024
0de7d32
Merge branch 'ISafe' of https://github.com/safe-global/safe-contracts…
remedcu Jan 4, 2024
95e247d
abstract Enum replaced with interface Enum
remedcu Jan 5, 2024
e10bb96
Author added to ISafe
remedcu Jan 5, 2024
70e12c9
Updating author
remedcu Jan 9, 2024
0ccc4a2
Created ISafeExtended by splitting ISafe
remedcu Jan 10, 2024
ce6e554
Harness patch updated
remedcu Jan 10, 2024
c46d5bd
Using harness patch to remove getTransactionHash(...) in ISafe
remedcu Jan 10, 2024
c637ab5
Creating nonce in ISafe and using ISafe for examples and libraries
remedcu Jan 10, 2024
1eb8e96
Merge branch 'main' into ISafe
remedcu Jan 12, 2024
f090084
Removed ISafeExtended and created individual interfaces
remedcu Jan 15, 2024
615383a
Removed Enum use in IGuardManager
remedcu Jan 15, 2024
7e7deeb
Enum file name changed
remedcu Jan 16, 2024
d765019
Changed Interface Enum to Library Enum
remedcu Jan 16, 2024
4e7649d
Adding approvedHashes into ISafe
remedcu Jan 16, 2024
bb1662e
Natspec for ISafe getter functions
remedcu Jan 16, 2024
33bca3d
Moved Events to Interfaces
remedcu Jan 16, 2024
2401405
Certora Harness Patch Updated
remedcu Jan 16, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contracts/Safe.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {Singleton} from "./common/Singleton.sol";
import {SignatureDecoder} from "./common/SignatureDecoder.sol";
import {SecuredTokenTransfer} from "./common/SecuredTokenTransfer.sol";
import {StorageAccessible} from "./common/StorageAccessible.sol";
import {Enum} from "./common/Enum.sol";
import {Enum} from "./interfaces/IEnum.sol";
import {ISignatureValidator, ISignatureValidatorConstants} from "./interfaces/ISignatureValidator.sol";
import {SafeMath} from "./external/SafeMath.sol";

Expand Down
2 changes: 1 addition & 1 deletion contracts/base/Executor.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;
import {Enum} from "../common/Enum.sol";
import {Enum} from "../interfaces/IEnum.sol";

/**
* @title Executor - A contract that can execute transactions
Expand Down
2 changes: 1 addition & 1 deletion contracts/base/GuardManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/* solhint-disable one-contract-per-file */
pragma solidity >=0.7.0 <0.9.0;

import {Enum} from "../common/Enum.sol";
import {Enum} from "../interfaces/IEnum.sol";
import {SelfAuthorized} from "../common/SelfAuthorized.sol";
import {IERC165} from "../interfaces/IERC165.sol";

Expand Down
2 changes: 1 addition & 1 deletion contracts/base/ModuleManager.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;
import {Enum} from "../common/Enum.sol";
import {Enum} from "../interfaces/IEnum.sol";
import {SelfAuthorized} from "../common/SelfAuthorized.sol";
import {Executor} from "./Executor.sol";
import {GuardManager, Guard} from "./GuardManager.sol";
Expand Down
2 changes: 1 addition & 1 deletion contracts/examples/guards/DebugTransactionGuard.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;

import {Enum} from "../../common/Enum.sol";
import {Enum} from "../../interfaces/IEnum.sol";
import {BaseGuard} from "../../base/GuardManager.sol";
import {Safe} from "../../Safe.sol";

Expand Down
2 changes: 1 addition & 1 deletion contracts/examples/guards/DelegateCallTransactionGuard.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;

import {Enum} from "../../common/Enum.sol";
import {Enum} from "../../interfaces/IEnum.sol";
import {BaseGuard} from "../../base/GuardManager.sol";

/**
Expand Down
7 changes: 2 additions & 5 deletions contracts/examples/guards/OnlyOwnersGuard.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@
/* solhint-disable one-contract-per-file */
pragma solidity >=0.7.0 <0.9.0;

import {Enum} from "../../common/Enum.sol";
import {Enum} from "../../interfaces/IEnum.sol";
import {BaseGuard} from "../../base/GuardManager.sol";

interface ISafe {
function isOwner(address owner) external view returns (bool);
}
import {ISafe} from "../../interfaces/ISafe.sol";

/**
* @title OnlyOwnersGuard - Only allows owners to execute transactions.
Expand Down
2 changes: 1 addition & 1 deletion contracts/examples/guards/ReentrancyTransactionGuard.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;

import {Enum} from "../../common/Enum.sol";
import {Enum} from "../../interfaces/IEnum.sol";
import {BaseGuard} from "../../base/GuardManager.sol";

/**
Expand Down
14 changes: 7 additions & 7 deletions contracts/handler/CompatibilityFallbackHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pragma solidity >=0.7.0 <0.9.0;

import {TokenCallbackHandler} from "./TokenCallbackHandler.sol";
import {ISignatureValidator} from "../interfaces/ISignatureValidator.sol";
import {Safe} from "../Safe.sol";
import {ISafe} from "../interfaces/ISafe.sol";
Copy link
Member

Choose a reason for hiding this comment

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

Could we import it as Safe? then it would not trigger any additional code changes (same was done for Enum)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think keeping it with an I is significant to notify the user that it is an interface and to follow a usual standard of interfaces. Also, using the same name will lead to HardhatError: HH701: There are multiple artifacts for contract "Safe", please use a fully qualified name. in tests (though it could be avoided by using the entire path to the contract I believe).

For Enum, the decision was taken to avoid the changes within the contract like Enum.Operation operation -> IEnum.Operation operation

import {HandlerContext} from "./HandlerContext.sol";

/**
Expand All @@ -24,7 +24,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
* @return Message hash.
*/
function getMessageHash(bytes memory message) public view returns (bytes32) {
return getMessageHashForSafe(Safe(payable(msg.sender)), message);
return getMessageHashForSafe(ISafe(payable(msg.sender)), message);
}

/**
Expand All @@ -33,7 +33,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
* @param message Message that should be encoded.
* @return Encoded message.
*/
function encodeMessageDataForSafe(Safe safe, bytes memory message) public view returns (bytes memory) {
function encodeMessageDataForSafe(ISafe safe, bytes memory message) public view returns (bytes memory) {
bytes32 safeMessageHash = keccak256(abi.encode(SAFE_MSG_TYPEHASH, keccak256(message)));
return abi.encodePacked(bytes1(0x19), bytes1(0x01), safe.domainSeparator(), safeMessageHash);
}
Expand All @@ -44,7 +44,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
* @param message Message that should be hashed.
* @return Message hash.
*/
function getMessageHashForSafe(Safe safe, bytes memory message) public view returns (bytes32) {
function getMessageHashForSafe(ISafe safe, bytes memory message) public view returns (bytes32) {
return keccak256(encodeMessageDataForSafe(safe, message));
}

Expand All @@ -56,7 +56,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
*/
function isValidSignature(bytes32 _dataHash, bytes calldata _signature) public view override returns (bytes4) {
// Caller should be a Safe
Safe safe = Safe(payable(msg.sender));
ISafe safe = ISafe(payable(msg.sender));
bytes memory messageData = encodeMessageDataForSafe(safe, abi.encode(_dataHash));
bytes32 messageHash = keccak256(messageData);
if (_signature.length == 0) {
Expand All @@ -73,7 +73,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
*/
function getModules() external view returns (address[] memory) {
// Caller should be a Safe
Safe safe = Safe(payable(msg.sender));
ISafe safe = ISafe(payable(msg.sender));
(address[] memory array, ) = safe.getModulesPaginated(SENTINEL_MODULES, 10);
return array;
}
Expand Down Expand Up @@ -174,6 +174,6 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
bytes memory signatures,
uint256 requiredSignatures
) public view {
Safe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, signatures, requiredSignatures);
ISafe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, signatures, requiredSignatures);
}
}
4 changes: 2 additions & 2 deletions contracts/common/Enum.sol → contracts/interfaces/IEnum.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ pragma solidity >=0.7.0 <0.9.0;

/**
* @title Enum - Collection of enums used in Safe contracts.
* @author Richard Meissner - @rmeissner
* @author Shebin John - @remedcu
remedcu marked this conversation as resolved.
Show resolved Hide resolved
*/
abstract contract Enum {
interface Enum {
remedcu marked this conversation as resolved.
Show resolved Hide resolved
enum Operation {
Call,
DelegateCall
Expand Down
146 changes: 146 additions & 0 deletions contracts/interfaces/ISafe.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;

import {Enum} from "./IEnum.sol";

/**
* @title ISafe - A multisignature wallet interface with support for confirmations using signed messages based on EIP-712.
* @author Shebin John - @remedcu
*/
interface ISafe {
/**
* @notice Sets an initial storage of the Safe contract.
* @dev This method can only be called once.
* If a proxy was created without setting up, anyone can call setup and claim the proxy.
* @param _owners List of Safe owners.
* @param _threshold Number of required confirmations for a Safe transaction.
* @param to Contract address for optional delegate call.
* @param data Data payload for optional delegate call.
* @param fallbackHandler Handler for fallback calls to this contract
* @param paymentToken Token that should be used for the payment (0 is ETH)
* @param payment Value that should be paid
* @param paymentReceiver Address that should receive the payment (or 0 if tx.origin)
*/
function setup(
remedcu marked this conversation as resolved.
Show resolved Hide resolved
address[] calldata _owners,
uint256 _threshold,
address to,
bytes calldata data,
address fallbackHandler,
address paymentToken,
uint256 payment,
address payable paymentReceiver
) external;

/** @notice Executes a `operation` {0: Call, 1: DelegateCall}} transaction to `to` with `value` (Native Currency)
* and pays `gasPrice` * `gasLimit` in `gasToken` token to `refundReceiver`.
* @dev The fees are always transferred, even if the user transaction fails.
* This method doesn't perform any sanity check of the transaction, such as:
* - if the contract at `to` address has code or not
* - if the `gasToken` is a contract or not
* It is the responsibility of the caller to perform such checks.
* @param to Destination address of Safe transaction.
* @param value Ether value of Safe transaction.
* @param data Data payload of Safe transaction.
* @param operation Operation type of Safe transaction.
* @param safeTxGas Gas that should be used for the Safe transaction.
* @param baseGas Gas costs that are independent of the transaction execution(e.g. base transaction fee, signature check, payment of the refund)
* @param gasPrice Gas price that should be used for the payment calculation.
* @param gasToken Token address (or 0 if ETH) that is used for the payment.
* @param refundReceiver Address of receiver of gas payment (or 0 if tx.origin).
* @param signatures Signature data that should be verified.
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
* @return success Boolean indicating transaction's success.
*/
function execTransaction(
address to,
uint256 value,
bytes calldata data,
Enum.Operation operation,
uint256 safeTxGas,
uint256 baseGas,
uint256 gasPrice,
address gasToken,
address payable refundReceiver,
bytes memory signatures
) external payable returns (bool success);

/**
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
* @param signatures Signature data that should be verified.
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
*/
function checkSignatures(bytes32 dataHash, bytes memory signatures) external view;

/**
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
* @dev Since the EIP-1271 does an external call, be mindful of reentrancy attacks.
* @param executor Address that executing the transaction.
* ⚠️⚠️⚠️ Make sure that the executor address is a legitmate executor.
* Incorrectly passed the executor might reduce the threshold by 1 signature. ⚠️⚠️⚠️
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
* @param signatures Signature data that should be verified.
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
* @param requiredSignatures Amount of required valid signatures.
*/
function checkNSignatures(address executor, bytes32 dataHash, bytes memory signatures, uint256 requiredSignatures) external view;

/**
* @notice Marks hash `hashToApprove` as approved.
* @dev This can be used with a pre-approved hash transaction signature.
* IMPORTANT: The approved hash stays approved forever. There's no revocation mechanism, so it behaves similarly to ECDSA signatures
* @param hashToApprove The hash to mark as approved for signatures that are verified by this contract.
*/
function approveHash(bytes32 hashToApprove) external;

/**
* @dev Returns the domain separator for this contract, as defined in the EIP-712 standard.
* @return bytes32 The domain separator hash.
*/
function domainSeparator() external view returns (bytes32);

/**
* @notice Returns transaction hash to be signed by owners.
* @param to Destination address.
* @param value Ether value.
* @param data Data payload.
* @param operation Operation type.
* @param safeTxGas Gas that should be used for the safe transaction.
* @param baseGas Gas costs for data used to trigger the safe transaction.
* @param gasPrice Maximum gas price that should be used for this transaction.
* @param gasToken Token address (or 0 if ETH) that is used for the payment.
* @param refundReceiver Address of receiver of gas payment (or 0 if tx.origin).
* @param _nonce Transaction nonce.
* @return Transaction hash.
*/
function getTransactionHash(
address to,
uint256 value,
bytes calldata data,
Enum.Operation operation,
uint256 safeTxGas,
uint256 baseGas,
uint256 gasPrice,
address gasToken,
address refundReceiver,
uint256 _nonce
) external view returns (bytes32);

/**
* @dev External getter function for state variables.
remedcu marked this conversation as resolved.
Show resolved Hide resolved
*/
// solhint-disable-next-line
function VERSION() external view returns (string memory);
remedcu marked this conversation as resolved.
Show resolved Hide resolved
function signedMessages(bytes32 messageHash) external view returns (uint256);

/**
* @dev External getter function for inherited functions.
*/
function getModulesPaginated(address start, uint256 pageSize) external view returns (address[] memory array, address next);
function getThreshold() external view returns (uint256);
function isOwner(address owner) external view returns (bool);
function getOwners() external view returns (address[] memory);
function setFallbackHandler(address handler) external;
function setGuard(address guard) external;
}
remedcu marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 1 addition & 4 deletions contracts/libraries/Safe130To141Migration.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@
pragma solidity >=0.7.0 <0.9.0;

import {SafeStorage} from "../libraries/SafeStorage.sol";

interface ISafe {
function setFallbackHandler(address handler) external;
}
import {ISafe} from "../interfaces/ISafe.sol";

/**
* @title Migration Contract for Safe Upgrade
Expand Down
8 changes: 1 addition & 7 deletions contracts/libraries/Safe150Migration.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,7 @@ pragma solidity >=0.7.0 <0.9.0;

import {SafeStorage} from "../libraries/SafeStorage.sol";
import {Guard} from "../base/GuardManager.sol";

// Interface for interacting with the Safe contract
interface ISafe {
function setFallbackHandler(address handler) external;

function setGuard(address guard) external;
}
import {ISafe} from "../interfaces/ISafe.sol";

/**
* @title Migration Contract for Safe Upgrade
Expand Down
14 changes: 2 additions & 12 deletions contracts/libraries/SafeToL2Migration.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,8 @@
pragma solidity >=0.7.0 <0.9.0;

import {SafeStorage} from "../libraries/SafeStorage.sol";
import {Enum} from "../common/Enum.sol";

interface ISafe {
// solhint-disable-next-line
function VERSION() external view returns (string memory);

function setFallbackHandler(address handler) external;

function getOwners() external view returns (address[] memory);

function getThreshold() external view returns (uint256);
}
import {Enum} from "../interfaces/IEnum.sol";
import {ISafe} from "../interfaces/ISafe.sol";

/**
* @title Migration Contract for updating a Safe from 1.1.1/1.3.0/1.4.1 versions to a L2 version. Useful when replaying a Safe from a non L2 network in a L2 network.
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"test": "hardhat test",
"test:parallel": "hardhat test --parallel",
"coverage": "hardhat coverage",
"codesize": "hardhat codesize",
remedcu marked this conversation as resolved.
Show resolved Hide resolved
"benchmark": "npm run test benchmark/*.ts",
"deploy-custom": "rm -rf deployments/custom && npm run deploy-all custom",
"deploy-all": "hardhat deploy-contracts --network",
Expand Down
Loading