diff --git a/contracts/src/hyperboards/WalletImpl.sol b/contracts/src/hyperboards/WalletImpl.sol index c4118d98..e493bef8 100644 --- a/contracts/src/hyperboards/WalletImpl.sol +++ b/contracts/src/hyperboards/WalletImpl.sol @@ -3,7 +3,6 @@ pragma solidity >=0.7.0 <0.9.0; import { Guard } from "safe-contracts/base/GuardManager.sol"; import { ModuleManager } from "safe-contracts/base/ModuleManager.sol"; -import { OwnerManager } from "safe-contracts/base/OwnerManager.sol"; import { FallbackManager } from "safe-contracts/base/FallbackManager.sol"; import { NativeCurrencyPaymentFallback } from "safe-contracts/common/NativeCurrencyPaymentFallback.sol"; import { Singleton } from "safe-contracts/common/Singleton.sol"; @@ -13,6 +12,7 @@ import { StorageAccessible } from "safe-contracts/common/StorageAccessible.sol"; import { Enum } from "safe-contracts/common/Enum.sol"; import { ISignatureValidator, ISignatureValidatorConstants } from "safe-contracts/interfaces/ISignatureValidator.sol"; import { SafeMath } from "safe-contracts/external/SafeMath.sol"; +import { Ownable } from "oz-contracts/contracts/access/Ownable.sol"; /** * @title Safe - A multisignature wallet with support for confirmations using signed messages based on EIP-712. @@ -35,7 +35,7 @@ contract Safe is Singleton, NativeCurrencyPaymentFallback, ModuleManager, - OwnerManager, + Ownable, SignatureDecoder, SecuredTokenTransfer, ISignatureValidatorConstants, @@ -50,14 +50,13 @@ contract Safe is // keccak256( // "EIP712Domain(uint256 chainId,address verifyingContract)" // ); - bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH =// solhint-disable-line - - 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218; + bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH = + 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218; // keccak256( // "SafeTx(address to,uint256 value,bytes data,uint8 operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,uint256 nonce)" // ); - bytes32 private constant SAFE_TX_TYPEHASH = 0xbb8310d486368db6bd6f849402fdd73ad53d316b5a4b2644ad6efe0f941286d8; // solhint-disable-line + bytes32 private constant SAFE_TX_TYPEHASH = 0xbb8310d486368db6bd6f849402fdd73ad53d316b5a4b2644ad6efe0f941286d8; event SafeSetup( address indexed initiator, @@ -79,46 +78,31 @@ contract Safe is mapping(address => mapping(bytes32 => uint256)) public approvedHashes; // This constructor ensures that this contract can only be used as a singleton for Proxy contracts - constructor() { - /** - * By setting the threshold it is not possible to call setup anymore, - * so we create a Safe with 0 owners and threshold 1. - * This is an unusable Safe, perfect for the singleton - */ - threshold = 1; - } /** * @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) - * @param useMsgSender use message.sender to verify transaction instead of signer, usefull for smart contract wallets */ function setup( - address[] calldata _owners, - uint256 _threshold, address to, bytes calldata data, address fallbackHandler, address paymentToken, uint256 payment, address payable paymentReceiver, - bool useMsgSender ) external { + // setupOwners checks if the Threshold is already set, therefore preventing that this method is called twice - setupOwners(_owners, _threshold); if (fallbackHandler != address(0)) internalSetFallbackHandler(fallbackHandler); // As setupOwners can only be called if the contract has not been initialized we don't need a check for setupModules setupModules(to, data); - _useMsgSnder = useMsgSender; if (payment > 0) { // To avoid running into issues with EIP-170 we reuse the _handlePayment function (to avoid adjusting code of that has been verified we do not adjust the method itself) // baseGas = 0, gasPrice = 1 and gas = payment => amount = (payment + 0) * 1 = payment @@ -157,8 +141,7 @@ contract Safe is uint256 gasPrice, address gasToken, address payable refundReceiver, - bytes memory signatures - ) public payable virtual returns (bool success) { + ) public payable virtual onlyOwner returns (bool success) { bytes32 txHash; // Use scope here to limit variable lifetime and prevent `stack too deep` errors { @@ -177,7 +160,7 @@ contract Safe is // We use the post-increment here, so the current nonce value is used and incremented afterwards. nonce++ ); - checkSignatures(txHash, "", signatures); + } address guard = getGuard(); { @@ -195,7 +178,6 @@ contract Safe is gasToken, refundReceiver, // Signature info - signatures, msg.sender ); } @@ -257,129 +239,10 @@ contract Safe is } } - /** - * @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 data That should be signed (this is passed to an external validator contract) - * @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 data, bytes memory signatures) public view { - // Load threshold to avoid multiple storage loads - uint256 _threshold = threshold; - // Check that a threshold is set - require(_threshold > 0, "GS001"); - checkNSignatures(msg.sender, dataHash, data, signatures, _threshold); - } - - /** - * @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. - * The data parameter (bytes) is not used since v1.5.0 as it is not required anymore. Prior to v1.5.0, - * data parameter was used in contract signature validation flow using legacy EIP-1271. - * Version v1.5.0, uses dataHash parameter instead of data with updated EIP-1271 implementation. - * @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 /* data */, - bytes memory signatures, - uint256 requiredSignatures - ) public view { - // Check that the provided signature data is not too short - require(signatures.length >= requiredSignatures.mul(65), "GS020"); - // There cannot be an owner with address 0. - address lastOwner = address(0); - address currentOwner; - uint8 v; - bytes32 r; - bytes32 s; - uint256 i; - for (i = 0; i < requiredSignatures; i++) { - (v, r, s) = signatureSplit(signatures, i); - if (v == 0) { - // If v is 0 then it is a contract signature - // When handling contract signatures the address of the contract is encoded into r - currentOwner = address(uint160(uint256(r))); - - // 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"); - - // Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes) - 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; - /* solhint-disable no-inline-assembly */ - /// @solidity memory-safe-assembly - assembly { - contractSignatureLen := mload(add(add(signatures, s), 0x20)) - } - /* solhint-enable no-inline-assembly */ - require(uint256(s).add(32).add(contractSignatureLen) <= signatures.length, "GS023"); - // Check signature - bytes memory contractSignature; - /* solhint-disable no-inline-assembly */ - /// @solidity memory-safe-assembly - assembly { - // 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) - } - /* solhint-enable no-inline-assembly */ - require( - ISignatureValidator(currentOwner).isValidSignature(dataHash, contractSignature) == - EIP1271_MAGIC_VALUE, - "GS024" - ); - } else if (v == 1) { - // If v is 1 then it is an approved hash - // 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"); - } 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 - ); - } 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); - } - require( - currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS, - "GS026" - ); - lastOwner = currentOwner; - } - } - /** - * @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 { - require(owners[msg.sender] != address(0), "GS030"); - approvedHashes[msg.sender][hashToApprove] = 1; - emit ApproveHash(hashToApprove, msg.sender); - } /** * @dev Returns the domain separator for this contract, as defined in the EIP-712 standard.