diff --git a/README.md b/README.md index 0a32024..c2a5869 100644 --- a/README.md +++ b/README.md @@ -108,8 +108,10 @@ See [safe-deployments](https://github.com/safe-global/safe-deployments/tree/main #### Security Considerations -- Delegatecalls can modify Safe state if not properly restricted -- HSG validates that approved delegatecalls don't modify critical Safe parameters +- Delegatecalls can modify Safe state if not properly restricted. Owners should NOT approve delegatecall targets that enable the following: + - Directly modifying any of the Safe's state, including the Safe's nonce. + - Additional delegatecalls. For example, the MultiSend library that is *not* MultiSendCallOnly should not be approved. +- HSG validates that approved delegatecalls don't modify critical Safe parameters, but relies on the Safe' nonce to do so. - Direct calls to the Safe are always prohibited ### Contract Ownership diff --git a/src/HatsSignerGate.sol b/src/HatsSignerGate.sol index 13fbded..e097ad0 100644 --- a/src/HatsSignerGate.sol +++ b/src/HatsSignerGate.sol @@ -95,28 +95,31 @@ contract HatsSignerGate is /*////////////////////////////////////////////////////////////// TRANSIENT STATE //////////////////////////////////////////////////////////////*/ - + /// @dev Temporary record of the existing owners on the `safe` when a transaction is submitted bytes32 transient _existingOwnersHash; - + /// @dev Temporary record of the existing threshold on the `safe` when a transaction is submitted uint256 transient _existingThreshold; - + /// @dev Temporary record of the existing fallback handler on the `safe` when a transaction is submitted address transient _existingFallbackHandler; - + /// @dev Temporary record of the operation type when a transaction is submitted Enum.Operation transient _operation; - - /// @dev A simple re-entrancy guard - uint256 transient _reentrancyGuard; - + + /// @dev Temporary record of whether we're inside an execTransaction call + bool transient _inSafeExecTransaction; + + /// @dev Temporary record of whether we're inside an execTransactionFromModule call + bool transient _inModuleExecTransaction; + /// @dev The safe's nonce at the beginning of a transaction uint256 transient _initialNonce; - + /// @dev The number of times the checkTransaction function has been called in a transaction - uint256 transient _entrancyCounter; - + uint256 transient _checkTransactionCounter; + /*////////////////////////////////////////////////////////////// AUTHENTICATION FUNCTIONS //////////////////////////////////////////////////////////////*/ @@ -396,7 +399,9 @@ contract HatsSignerGate is //////////////////////////////////////////////////////////////*/ /// @inheritdoc BaseGuard - /// @notice Only approved delegatecall targets are allowed + /// @notice Only approved delegatecall targets are allowed. + /// @notice This function cannot be entered from within itself, called directly from within Safe.execTransaction, or + /// from within an execTransactionFromModule or execTransactionFromModuleReturnData call. function checkTransaction( address to, uint256 value, @@ -413,28 +418,20 @@ contract HatsSignerGate is // ensure that the call is coming from the safe if (msg.sender != address(safe)) revert NotCalledFromSafe(); - // if _reentrancyGuard equals 1 it means that there is an ongoing execution either from execTransactionFromModule or - // execTransactionFromModuleReturnData. - // this check prevents entering the checkTransaction in this case in order to avoid overriding the snapshot of the - // Safe state - if (_reentrancyGuard == 1) revert NoReentryAllowed(); - - // record the initial nonce of the safe at the beginning of the transaction - if (_entrancyCounter == 0) { - _initialNonce = safe.nonce() - 1; - } - - // increment the entrancy count - _entrancyCounter++; - - // Whenever this function is called from a source other than Safe.execTransaction, it`s possible that - // it is a malicious call attempting to manipulate the transient storage so that an attacker can - // make malicious changes to the Safe state without detection by this guard in - // IGuard.checkAfterExecution. To prevent this, we rely on the invariant that the Safe nonce - // increments every time Safe.execTransaction calls out to IGuard.checkTransaction, allowing us to - // ensure that this function is only called the same number of times in a single transaction as - // Safe.execTransaction calls, ie by how much the Safe's nonce increases. - if (safe.nonce() - _initialNonce != _entrancyCounter) revert NoReentryAllowed(); + // Disallow entering this function from a) inside a Safe.execTransaction call or b) inside an + // execTransactionFromModule call + if (_inSafeExecTransaction || _inModuleExecTransaction) revert NoReentryAllowed(); + _inSafeExecTransaction = true; + + /// @dev We enforce that this function is called exactly once per Safe.execTransaction call. Leveraging the fact + /// that the Safe nonce increments exactly once per Safe.execTransaction call, we require that nonce diff matches + /// the number of times this function has been called. + // Record the initial nonce of the Safe at the beginning of the transaction + if (_checkTransactionCounter == 0) _initialNonce = safe.nonce() - 1; + // Increment the entrancy counter + _checkTransactionCounter++; + // Ensure that this function is called exactly once per Safe.execTransaction call + if (safe.nonce() - _initialNonce != _checkTransactionCounter) revert NoReentryAllowed(); // module guard preflight check if (guard != address(0)) { @@ -450,7 +447,7 @@ contract HatsSignerGate is address(0), payable(0), "", - address(0) + address(safe) ); } @@ -511,6 +508,7 @@ contract HatsSignerGate is * 2. changing any modules * 3. changing the threshold * 4. changing the owners + * 5. changing the fallback handler * CAUTION: If the safe has any authority over the signersHat(s) — i.e. wears their admin hat(s) or is an * eligibility or toggle module — then in some cases protections (3) and (4) may not hold. Proceed with caution if * considering granting such authority to the safe. @@ -518,11 +516,18 @@ contract HatsSignerGate is * https://github.com/gnosis/zodiac-guard-mod/blob/988ebc7b71e352f121a0be5f6ae37e79e47a4541/contracts/ModGuard.sol#L86 */ function checkAfterExecution(bytes32, bool) public override { + // Ensure that this is only called in accordance with the Safe execTransaction flow + // And that it is not called from inside an execTransactionFromModule call + if (!_inSafeExecTransaction || _inModuleExecTransaction) revert NoReentryAllowed(); + // module guard postflight check if (guard != address(0)) { BaseGuard(guard).checkAfterExecution(bytes32(0), false); } + // reset the reentrancy guard to enable subsequent legitimate Safe execTransactions in the same transaction + _inSafeExecTransaction = false; + // if the transaction was a delegatecall, perform the post-flight check on the Safe state // we don't need to check the Safe state for regular calls since the Safe state cannot be altered except by calling // into the Safe, which is explicitly disallowed @@ -981,15 +986,11 @@ contract HatsSignerGate is /// @param operation_ The operation type of the module transaction /// @return _safe The safe that is executing the module transaction function _beforeExecTransactionFromModule(address _to, Enum.Operation operation_) internal returns (ISafe _safe) { - // The _entrancyCounter transient variable counts the number of times that the checkTransaction function - // was called in the current transaction. Calling checkTransaction prior to this function is unsafe since it may - // override the safe state snapshot, so we revert. - // The _reentrancyGuard tracks entrance into either execTransactionFromModule or execTransactionFromModuleReturnData. - // Reentering either of those functions is unsafe since it may override the safe state snapshot, so we revert. - if (_entrancyCounter > 0 || _reentrancyGuard == 1) revert NoReentryAllowed(); + // Disallow entering this function from inside a Safe execTransaction call or another execTransactionFromModule call + if (_inSafeExecTransaction || _inModuleExecTransaction) revert NoReentryAllowed(); // set the reentrancy guard - _reentrancyGuard = 1; + _inModuleExecTransaction = true; _safe = safe; @@ -1015,6 +1016,6 @@ contract HatsSignerGate is if (operation_ == Enum.Operation.DelegateCall) _checkSafeState(_safe); // reset the reentrancy guard to enable future legitimate calls within the same transaction - _reentrancyGuard = 0; + _inModuleExecTransaction = false; } } diff --git a/test/HatsSignerGate.attacks.t.sol b/test/HatsSignerGate.attacks.t.sol index 0239904..3646831 100644 --- a/test/HatsSignerGate.attacks.t.sol +++ b/test/HatsSignerGate.attacks.t.sol @@ -4,8 +4,30 @@ pragma solidity ^0.8.13; import { Test, console2 } from "../lib/forge-std/src/Test.sol"; import { WithHSGInstanceTest, Enum } from "./TestSuite.t.sol"; import { IHatsSignerGate } from "../src/interfaces/IHatsSignerGate.sol"; +import { SafeManagerLib } from "../src/lib/SafeManagerLib.sol"; contract AttacksScenarios is WithHSGInstanceTest { + address public maliciousFallbackHandler = makeAddr("maliciousFallbackHandler"); + address public goodFallbackHandler; + bytes public setFallbackAction = abi.encodeWithSignature("setFallbackHandler(address)", maliciousFallbackHandler); + bytes public packedCalls; + bytes public multiSendData; + bytes32 public safeTxHash; + bytes public checkTransactionAction; + bytes public signatures; + + bytes public checkAfterExecutionAction = + abi.encodeWithSignature("checkAfterExecution(bytes32,bool)", bytes32(0), false); + + string public constant CHECK_TRANSACTION_SIGNATURE = + "checkTransaction(address,uint256,bytes,uint8,uint256,uint256,uint256,address,address,bytes,address)"; + + function setUp() public override { + super.setUp(); + + goodFallbackHandler = SafeManagerLib.getSafeFallbackHandler(safe); + } + function testSignersCannotAddNewModules() public { _addSignersSameHat(2, signerHat); @@ -13,7 +35,7 @@ contract AttacksScenarios is WithHSGInstanceTest { (bytes memory multisendCall, bytes32 txHash) = _constructSingleActionMultiSendTx(addModuleData); - bytes memory signatures = _createNSigsForTx(txHash, 2); + signatures = _createNSigsForTx(txHash, 2); // execute tx, expecting a revert vm.expectRevert(IHatsSignerGate.CannotChangeModules.selector); @@ -67,7 +89,7 @@ contract AttacksScenarios is WithHSGInstanceTest { // create the tx bytes32 txHash = _getTxHash(destAddress, transferValue, Enum.Operation.Call, hex"00", safe); // have them sign it - bytes memory signatures = _createNSigsForTx(txHash, 2); + signatures = _createNSigsForTx(txHash, 2); vm.expectRevert(); safe.execTransaction( @@ -182,12 +204,12 @@ contract AttacksScenarios is WithHSGInstanceTest { address(0), // refundReceiver safe.nonce() // nonce ); - bytes memory sigs = _createNSigsForTx(txHash, 3); + signatures = _createNSigsForTx(txHash, 3); // attacker adds a contract signature from the 4th signer from a previous tx // since HSG doesn't check that the correct data was signed, it would be considered a valid signature bytes memory contractSig = abi.encode(signerAddresses[3], bytes32(0), bytes1(0x01)); - sigs = bytes.concat(sigs, contractSig); + signatures = bytes.concat(signatures, contractSig); // mock the maliciousTx so it would succeed if it were to be executed vm.mockCall(maliciousContract, maliciousTx, abi.encode(true)); @@ -206,7 +228,7 @@ contract AttacksScenarios is WithHSGInstanceTest { address(0), payable(address(0)), // (r,s,v) [r - from] [s - unused] [v - 1 flag for onchain approval] - sigs + signatures ); assertEq(safe.getThreshold(), 3, "post threshold"); @@ -214,12 +236,10 @@ contract AttacksScenarios is WithHSGInstanceTest { assertEq(safe.nonce(), 0, "post nonce hasn't changed"); } - function testSignersCannotReenterCheckTransactionToAddOwners() public { + function test_revert_reenterCheckTransaction() public { address newOwner = makeAddr("newOwner"); bytes memory addOwnerAction; - bytes memory sigs; bytes memory checkTxAction; - bytes memory multisend; // start with 3 valid signers _addSignersSameHat(3, signerHat); // attacker is the first of these signers @@ -260,7 +280,7 @@ contract AttacksScenarios is WithHSGInstanceTest { ); // then have it signed by the attacker and a collaborator - sigs = _createNSigsForTx(dummyTxHash, 2); + // sigs = checkTxAction = abi.encodeWithSelector( instance.checkTransaction.selector, @@ -274,12 +294,12 @@ contract AttacksScenarios is WithHSGInstanceTest { 0, address(0), payable(address(0)), - sigs, + _createNSigsForTx(dummyTxHash, 2), attacker // msgSender ); // now bundle the two actions into a multisend tx - bytes memory packedCalls = abi.encodePacked( + packedCalls = abi.encodePacked( // 1) add owner uint8(0), // 0 for call; 1 for delegatecall safe, // to @@ -293,14 +313,14 @@ contract AttacksScenarios is WithHSGInstanceTest { uint256(checkTxAction.length), // data length bytes(checkTxAction) // data ); - multisend = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); } // now get the safe tx hash and have attacker sign it with a collaborator - bytes32 safeTxHash = safe.getTransactionHash( + safeTxHash = safe.getTransactionHash( defaultDelegatecallTargets[0], // to an approved delegatecall target 0, // value - multisend, // data + multiSendData, // data Enum.Operation.DelegateCall, // operation 0, // safeTxGas 0, // baseGas @@ -309,7 +329,7 @@ contract AttacksScenarios is WithHSGInstanceTest { address(0), // refundReceiver safe.nonce() // nonce ); - sigs = _createNSigsForTx(safeTxHash, 2); + signatures = _createNSigsForTx(safeTxHash, 2); // now submit the tx to the safe vm.prank(attacker); @@ -324,7 +344,7 @@ contract AttacksScenarios is WithHSGInstanceTest { safe.execTransaction( defaultDelegatecallTargets[0], // to an approved delegatecall target 0, - multisend, + multiSendData, Enum.Operation.DelegateCall, // not using the refunder 0, @@ -332,10 +352,756 @@ contract AttacksScenarios is WithHSGInstanceTest { 0, address(0), payable(address(0)), - sigs + signatures ); // no new owners have been added, despite the attacker's best efforts assertEq(safe.getOwners().length, 3, "post owner count"); } + + function test_revert_callCheckTransactionFromMultisend() public { + // our scenario starts with HSG attached to a safe, with 3 valid signers and a threshold of 2 + _addSignersSameHat(3, signerHat); + + // the attacker crafts a multisend tx that contains two actions: + // 1) set the maliciousFallback as the fallback + // 2) calls Safe.execTransaction with valid signatures. This can be an empty tx; its just there to enter the + // guard functions to overwrite the snapshot so the outer call can pass the checks. + + // 1) set the maliciousFallbackHandler as the fallback + // bytes memory setFallbackAction = abi.encodeWithSignature("setFallbackHandler(address)", + // maliciousFallbackHandler); + + // 2) call Safe.execTransaction with valid signatures + // get the hash of the empty action + bytes32 emptyTransactionHash = safe.getTransactionHash( + address(signerAddresses[0]), // must be non-safe target + 0, + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + address(0), + safe.nonce() + 1 // nonce increments after the outer call to execTransaction + ); + // sufficient signers sign it + // bytes memory emptySigs = _createNSigsForTx(emptyTransactionHash, 2); + + // get the calldata + bytes memory emptyExecTransactionAction = abi.encodeWithSignature( + "execTransaction(address,uint256,bytes,uint8,uint256,uint256,uint256,address,address,bytes)", + address(signerAddresses[0]), // must be non-safe target + 0, + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + address(0), + _createNSigsForTx(emptyTransactionHash, 2) + ); + + // bundle the two actions into a multisend + packedCalls = abi.encodePacked( + // 1) setFallback + uint8(0), // 0 for call; 1 for delegatecall + safe, // to + uint256(0), // value + uint256(setFallbackAction.length), // data length + bytes(setFallbackAction), // data + // 2) execTransaction + uint8(0), // 0 for call; 1 for delegatecall + safe, // to + uint256(0), // value + uint256(emptyExecTransactionAction.length), // data length + bytes(emptyExecTransactionAction) // INNER action + ); + + // OUTER action + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + // get the safe tx hash and have the signers sign it + safeTxHash = safe.getTransactionHash( + defaultDelegatecallTargets[0], // to an approved delegatecall target + 0, + multiSendData, + Enum.Operation.DelegateCall, + 0, + 0, + 0, + address(0), + address(0), + safe.nonce() + ); + + signatures = _createNSigsForTx(safeTxHash, 2); + + // submit the tx to the safe, expecting a revert + vm.prank(signerAddresses[0]); + /* + Expect revert because of re-entry into checkTransaction + While instance will throw the NoReentryAllowed error, + since the error occurs within the context of the safe transaction, + the safe will catch the error and re-throw with its own error, + ie `GS013` ("Safe transaction failed when gasPrice and safeTxGas were 0") + */ + vm.expectRevert("GS013"); + safe.execTransaction( + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunder + 0, + 0, + 0, + address(0), + payable(address(0)), + signatures + ); + + // the fallback should not be different + address fallbackHandler = SafeManagerLib.getSafeFallbackHandler(safe); + assertEq(fallbackHandler, goodFallbackHandler, "fallbackHandler should be the same as before"); + } + + function test_revert_callCheckAfterExecutionInsideMultisend() public { + // start with 3 valid signers + _addSignersSameHat(3, signerHat); + + // the attacker crafts a multisend tx that contains three actions: + // 1) set the maliciousFallbackHandler as the fallback — this will set the _inExecTransaction flag to true + // 2) HSG.checkAfterExecution — this will reset the _inExecTransaction flag to false + // 3) HSG.checkTransaction — this will set the _inExecTransaction flag to back to true and overwrite the snapshot + + // 1) set the maliciousFallbackHandler as the fallback + // bytes memory setFallbackAction = abi.encodeWithSignature("setFallbackHandler(address)", + // maliciousFallbackHandler); + + // 2) HSG.checkAfterExecution + // bytes memory checkAfterExecutionAction = + // abi.encodeWithSignature("checkAfterExecution(bytes32,bool)", bytes32(0), false); + + // 3) HSG.checkTransaction + { + checkTransactionAction = abi.encodeWithSignature( + CHECK_TRANSACTION_SIGNATURE, + address(0), + 0, + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + address(0), + _createNContractSigs(2), // attacker's spoofed signatures + address(safe) + ); + + // bundle the three actions into a multisend + packedCalls = abi.encodePacked( + // 1) setFallback + uint8(0), // 0 for call; 1 for delegatecall + address(safe), // to + uint256(0), // value + uint256(setFallbackAction.length), // data length + bytes(setFallbackAction), // data + // 2) checkAfterExecution + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkAfterExecutionAction.length), // data length + bytes(checkAfterExecutionAction) // data + ); + + // workaround to avoid stack too deep error + packedCalls = abi.encodePacked( + packedCalls, + // 3) checkTransaction + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkTransactionAction.length), // data length + bytes(checkTransactionAction) // data + ); + + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + safeTxHash = _getSafeDelegatecallHash(defaultDelegatecallTargets[0], multiSendData, safe); + + signatures = _createNSigsForTx(safeTxHash, 2); + } + + // submit the tx to the safe, expecting a revert + vm.expectRevert("GS013"); + safe.execTransaction( + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunder + 0, + 0, + 0, + address(0), + payable(address(0)), + signatures + ); + + // the fallback should not be different + assertEq( + SafeManagerLib.getSafeFallbackHandler(safe), goodFallbackHandler, "fallbackHandler should be the same as before" + ); + } + + function test_revert_callCheckAfterExecutionFromMultisend() public { + // our scenario starts with HSG attached to a safe, with 3 valid signers and a threshold of 2 + _addSignersSameHat(3, signerHat); + + // the attacker crafts a multisend tx that contains two actions: + // 1) HSG.checkAfterExecution — this will reset the _inSafeExecTransaction flag to false after it was set to + // true in checkTransaction + // 2) set the maliciousFallbackHandler as the fallback + // 3) Safe.execTransaction + + // 1) HSG.checkAfterExecution + // bytes memory checkAfterExecutionAction = + // abi.encodeWithSignature("checkAfterExecution(bytes32,bool)", bytes32(0), false); + + // 2) set the maliciousFallbackHandler as the fallback + // bytes memory setFallbackAction = abi.encodeWithSignature("setFallbackHandler(address)", + // maliciousFallbackHandler); + + // 3) call Safe.execTransaction with valid signatures + // get the hash of the empty action + bytes32 emptyTransactionHash = safe.getTransactionHash( + address(signerAddresses[0]), // must be non-safe target + 0, + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + address(0), + safe.nonce() + 1 // nonce increments after the outer call to execTransaction + ); + // sufficient signers sign it + bytes memory emptySigs = _createNSigsForTx(emptyTransactionHash, 2); + + // get the calldata + bytes memory emptyExecTransactionAction = abi.encodeWithSignature( + "execTransaction(address,uint256,bytes,uint8,uint256,uint256,uint256,address,address,bytes)", + address(signerAddresses[0]), // must be non-safe target + 0, + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + address(0), + emptySigs + ); + + // bundle the three actions into a multisend + packedCalls = abi.encodePacked( + // checkTransaction + // 1) checkAfterExecution + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkAfterExecutionAction.length), // data length + bytes(checkAfterExecutionAction), // data + // 2) setFallback + uint8(0), // 0 for call; 1 for delegatecall + address(safe), // to + uint256(0), // value + uint256(setFallbackAction.length), // data length + bytes(setFallbackAction) // data + ); + + // workaround to avoid stack too deep error + packedCalls = abi.encodePacked( + packedCalls, + // 3) execTransaction + // checkTransaction + uint8(0), // 0 for call; 1 for delegatecall + address(safe), // to + uint256(0), // value + uint256(emptyExecTransactionAction.length), // data length + bytes(emptyExecTransactionAction) // data + // checkAfterExecution + // checkAfterExecution + ); + + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + // get the safe tx hash and have the signers sign it + safeTxHash = safe.getTransactionHash( + defaultDelegatecallTargets[0], // to an approved delegatecall target + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunder + 0, + 0, + 0, + address(0), + address(0), + safe.nonce() + ); + + signatures = _createNSigsForTx(safeTxHash, 2); + + // submit the tx to the safe, expecting a revert + vm.prank(signerAddresses[0]); + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + safe.execTransaction( + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunder + 0, + 0, + 0, + address(0), + payable(address(0)), + signatures + ); + + // the fallback should be different + assertEq( + SafeManagerLib.getSafeFallbackHandler(safe), goodFallbackHandler, "fallbackHandler should be the same as before" + ); + } + + function test_revert_bypassHSGGuardByDisablingHSG() public { + // start with 3 valid signers + _addSignersSameHat(3, signerHat); + + // the attacker crafts a multisend tx that contains three actions: + // 1) HSG.checkAfterExecution — this will reset the _inSafeExecTransaction flag to false after it was set to + // true in the outer call + // 2) Safe.setFallbackHandler to set the maliciousFallbackHandler as the fallback + // 3) HSG.checkTransaction — this will set the _inSafeExecTransaction flag to back to true and overwrite the + // snapshot + + // 1) HSG.checkAfterExecution + // bytes memory checkAfterExecutionAction = + // abi.encodeWithSignature("checkAfterExecution(bytes32,bool)", bytes32(0), false); + + // 2) Safe.setFallbackHandler to set the maliciousFallbackHandler as the fallback + // bytes memory setFallbackAction = abi.encodeWithSignature("setFallbackHandler(address)", + // maliciousFallbackHandler); + + // 3) HSG.checkTransaction + checkTransactionAction = abi.encodeWithSignature( + CHECK_TRANSACTION_SIGNATURE, + address(0), + 0, + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + address(0), + _createNContractSigs(2), // attacker's spoofed signatures + address(safe) + ); + + // bundle the three actions into a multisend + packedCalls = abi.encodePacked( + // 1) checkAfterExecution + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkAfterExecutionAction.length), // data length + bytes(checkAfterExecutionAction), // data + // 2) setFallback + uint8(0), // 0 for call; 1 for delegatecall + address(safe), // to + uint256(0), // value + uint256(setFallbackAction.length), // data length + bytes(setFallbackAction) // data + ); + + // workaround to avoid stack too deep error + packedCalls = abi.encodePacked( + packedCalls, + // 3) checkTransaction + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkTransactionAction.length), // data length + bytes(checkTransactionAction) // data + ); + + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + // get the safe tx hash and have the signers sign it + safeTxHash = safe.getTransactionHash( + defaultDelegatecallTargets[0], // to an approved delegatecall target + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunders + 0, + 0, + 0, + address(0), + address(0), + safe.nonce() + ); + + // signers sign the tx + bytes memory sigs = _createNSigsForTx(safeTxHash, 2); + + // submit the tx to the safe, expecting a revert + vm.prank(signerAddresses[0]); + vm.expectRevert("GS013"); + safe.execTransaction( + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunder + 0, + 0, + 0, + address(0), + payable(address(0)), + sigs + ); + + // the fallback should be different + assertEq( + SafeManagerLib.getSafeFallbackHandler(safe), goodFallbackHandler, "fallbackHandler should be the same as before" + ); + } + + function test_revert_callExecTransactionFromModuleInsideMultisend() public { + // start with 3 valid signers + _addSignersSameHat(3, signerHat); + + // the attacker crafts a multisend tx that contains four actions: + // 1) HSG.checkAfterExecution — this will reset the _inSafeExecTransaction flag to false after it was set to + // true in checkTransaction + // 2) Safe.setFallbackHandler to set the maliciousFallbackHandler as the fallback + // 3) HSG.execTransactionFromModule — this will update the Safe state snapshot + // But the outer call to HSG.checkAfterExecution will revert because the _inSafeExecTransaction flag is false + + // simplest version of (3) requires that the safe has somehow been enabled as a module on HSG. Otherwise, it will + // revert with a NotAuthorized() error. + vm.prank(owner); + instance.enableModule(address(safe)); + + // (3) craft an empty execTransactionFromModule call + bytes memory execTransactionFromModuleAction = abi.encodeWithSignature( + "execTransactionFromModule(address,uint256,bytes,uint8)", address(0), 0, hex"", Enum.Operation.Call + ); + + // 4) HSG.checkTransaction + checkTransactionAction = abi.encodeWithSignature( + CHECK_TRANSACTION_SIGNATURE, + address(0), + 0, + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + address(0), + _createNContractSigs(2), // attacker's spoofed signatures + address(safe) + ); + + // bundle the three actions into a multisend + packedCalls = abi.encodePacked( + // 1) checkAfterExecution + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkAfterExecutionAction.length), // data length + bytes(checkAfterExecutionAction), // data + // 2) setFallback + uint8(0), // 0 for call; 1 for delegatecall + address(safe), // to + uint256(0), // value + uint256(setFallbackAction.length), // data length + bytes(setFallbackAction) // data + ); + + // workaround to avoid stack too deep error + packedCalls = abi.encodePacked( + packedCalls, + // 3) execTransactionFromModule + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(execTransactionFromModuleAction.length), // data length + bytes(execTransactionFromModuleAction) // data + ); + + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + // get the safe tx hash + safeTxHash = safe.getTransactionHash( + defaultDelegatecallTargets[0], // to an approved delegatecall target + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunders + 0, + 0, + 0, + address(0), + address(0), + safe.nonce() + ); + + // signers sign the tx + signatures = _createNSigsForTx(safeTxHash, 2); + + // submit the tx to the safe, expecting a revert + vm.prank(signerAddresses[0]); + // since the revert comes from the outer call to HSG.checkAfterExecution, we expect NoReentryAllowed because the + // call to checkAfterExecution comes after the error catching in Safe.execTransaction + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + safe.execTransaction( + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunders + 0, + 0, + 0, + address(0), + payable(address(0)), + signatures + ); + + // the fallback should the same + assertEq( + SafeManagerLib.getSafeFallbackHandler(safe), goodFallbackHandler, "fallbackHandler should be the same as before" + ); + } + + function test_revert_callExecTransactionFromModuleInsideMultisendWithCheckTransaction() public { + // start with 3 valid signers + _addSignersSameHat(3, signerHat); + + // the attacker crafts a multisend tx that contains four actions: + // 1) HSG.checkAfterExecution — this will reset the _inSafeExecTransaction flag to false after it was set to + // true in checkTransaction + // 2) Safe.setFallbackHandler to set the maliciousFallbackHandler as the fallback + // 3) HSG.execTransactionFromModule — this will update the Safe state snapshot + // 4) HSG.checkTransaction — this will set the _inSafeExecTransaction flag to true. But this should revert + // because the nonce checks prevent checkTransaction from being called more than once + + // simplest version of (3) requires that the safe has somehow been enabled as a module on HSG. Otherwise, it will + // revert with a NotAuthorized() error. + vm.prank(owner); + instance.enableModule(address(safe)); + + // (3) craft an empty execTransactionFromModule call + bytes memory execTransactionFromModuleAction = abi.encodeWithSignature( + "execTransactionFromModule(address,uint256,bytes,uint8)", address(0), 0, hex"", Enum.Operation.Call + ); + + // 4) HSG.checkTransaction + checkTransactionAction = abi.encodeWithSignature( + CHECK_TRANSACTION_SIGNATURE, + address(0), + 0, + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + address(0), + _createNContractSigs(2), // attacker's spoofed signatures + address(safe) + ); + + // bundle the three actions into a multisend + packedCalls = abi.encodePacked( + // 1) checkAfterExecution + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkAfterExecutionAction.length), // data length + bytes(checkAfterExecutionAction), // data + // 2) setFallback + uint8(0), // 0 for call; 1 for delegatecall + address(safe), // to + uint256(0), // value + uint256(setFallbackAction.length), // data length + bytes(setFallbackAction) // data + ); + + // workaround to avoid stack too deep error + packedCalls = abi.encodePacked( + packedCalls, + // 3) execTransactionFromModule + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(execTransactionFromModuleAction.length), // data length + bytes(execTransactionFromModuleAction), // data + // 4) checkTransaction + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkTransactionAction.length), // data length + bytes(checkTransactionAction) // data + ); + + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + // get the safe tx hash + safeTxHash = safe.getTransactionHash( + defaultDelegatecallTargets[0], // to an approved delegatecall target + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunders + 0, + 0, + 0, + address(0), + address(0), + safe.nonce() + ); + + // signers sign the tx + signatures = _createNSigsForTx(safeTxHash, 2); + + // submit the tx to the safe, expecting a revert + vm.prank(signerAddresses[0]); + // We expect GS013 since Safe.execTransaction catches the error NoReentryAllowed error thrown by + // HSG.checkTransaction + vm.expectRevert("GS013"); + safe.execTransaction( + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunders + 0, + 0, + 0, + address(0), + payable(address(0)), + signatures + ); + + // the fallback should the same + assertEq( + SafeManagerLib.getSafeFallbackHandler(safe), goodFallbackHandler, "fallbackHandler should be the same as before" + ); + } + + function test_revert_changeFallbackHandlerViaExecTransactionFromModuleInsideMultisend() public { + // start with 3 valid signers + _addSignersSameHat(3, signerHat); + + // the attacker crafts a multisend tx that contains two actions: + // 1) HSG.checkAfterExecution — this will reset the _inSafeExecTransaction flag to false after it was set to + // true in checkTransaction + // 2) HSG.execTransactionFromModule with a payload that changes the fallback handler inside of a multisend. This + // should revert because changing safe state is not allowed. + + // simplest version of (2) requires that the safe has somehow been enabled as a module on HSG. Otherwise, it will + // revert with a NotAuthorized() error. + vm.prank(owner); + instance.enableModule(address(safe)); + + // (2) craft the execTransactionFromModule action + packedCalls = abi.encodePacked( + uint8(0), // 0 for call; 1 for delegatecall + address(safe), // to + uint256(0), // value + uint256(setFallbackAction.length), // data length + bytes(setFallbackAction) // data + ); + + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + bytes memory execTransactionFromModuleAction = abi.encodeWithSignature( + "execTransactionFromModule(address,uint256,bytes,uint8)", + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall + ); + + // bundle the two actions into a multisend + packedCalls = abi.encodePacked( + // 1) checkAfterExecution + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(checkAfterExecutionAction.length), // data length + bytes(checkAfterExecutionAction), // data + // 2) execTransactionFromModule + uint8(0), // 0 for call; 1 for delegatecall + address(instance), // to + uint256(0), // value + uint256(execTransactionFromModuleAction.length), // data length + bytes(execTransactionFromModuleAction) // data + ); + + multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + // get the safe tx hash + safeTxHash = safe.getTransactionHash( + defaultDelegatecallTargets[0], // to an approved delegatecall target + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunders + 0, + 0, + 0, + address(0), + address(0), + safe.nonce() + ); + + // signers sign the tx + signatures = _createNSigsForTx(safeTxHash, 2); + + // submit the tx to the safe, expecting a revert + // We expect GS013 since Safe.execTransaction catches the CannotChangeFallbackHandler error thrown by + // HSG.execTransactionFromModule + vm.expectRevert("GS013"); + safe.execTransaction( + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall, + // not using the refunders + 0, + 0, + 0, + address(0), + payable(address(0)), + signatures + ); + + // the fallback should the same + assertEq( + SafeManagerLib.getSafeFallbackHandler(safe), goodFallbackHandler, "fallbackHandler should be the same as before" + ); + } } diff --git a/test/HatsSignerGate.moduleTxs.sol b/test/HatsSignerGate.moduleTxs.sol index 22b6a4f..14aa889 100644 --- a/test/HatsSignerGate.moduleTxs.sol +++ b/test/HatsSignerGate.moduleTxs.sol @@ -2,14 +2,15 @@ pragma solidity ^0.8.13; import { Test, console2 } from "../lib/forge-std/src/Test.sol"; -import { Enum, WithHSGInstanceTest } from "./TestSuite.t.sol"; +import { Enum, WithHSGInstanceTest, WithHSGHarnessInstanceTest } from "./TestSuite.t.sol"; import { IHats, IHatsSignerGate } from "../src/interfaces/IHatsSignerGate.sol"; +import { SafeManagerLib } from "../src/lib/SafeManagerLib.sol"; import { IAvatar } from "../src/lib/zodiac-modified/ModifierUnowned.sol"; import { IModuleManager } from "../src/lib/safe-interfaces/IModuleManager.sol"; import { ModifierUnowned } from "../src/lib/zodiac-modified/ModifierUnowned.sol"; import { MultiSend } from "../lib/safe-smart-account/contracts/libraries/MultiSend.sol"; -contract ExecutingFromModuleViaHSG is WithHSGInstanceTest { +contract ExecutingFromModuleViaHSG is WithHSGHarnessInstanceTest { address newModule = tstModule1; address recipient = makeAddr("recipient"); @@ -18,66 +19,139 @@ contract ExecutingFromModuleViaHSG is WithHSGInstanceTest { // enable a new module vm.prank(owner); - instance.enableModule(newModule); + harness.enableModule(newModule); // deal the safe some eth deal(address(safe), 1 ether); } - function test_happy_executionSuccess() public { + function test_happy_executionSuccess() public inSafeExecTransaction(false) inModuleExecTransaction(false) { uint256 preValue = address(safe).balance; uint256 transferValue = 0.3 ether; uint256 postValue = preValue - transferValue; // have the new module submit/exec the tx, expecting a success event emission from both hsg and the newModule vm.expectEmit(); - emit IModuleManager.ExecutionFromModuleSuccess(address(instance)); + emit IModuleManager.ExecutionFromModuleSuccess(address(harness)); vm.expectEmit(); emit IAvatar.ExecutionFromModuleSuccess(newModule); vm.prank(newModule); - instance.execTransactionFromModule(recipient, transferValue, hex"00", Enum.Operation.Call); + harness.execTransactionFromModule(recipient, transferValue, hex"00", Enum.Operation.Call); // confirm the tx succeeded by checking ETH balance changes assertEq(address(safe).balance, postValue); assertEq(recipient.balance, transferValue); + + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_happy_executionFailure() public { + function test_happy_executionFailure() public inSafeExecTransaction(false) inModuleExecTransaction(false) { // craft a call to a function that doesn't exist on a contract (we'll use Hats.sol) bytes memory badCall = abi.encodeWithSignature("badCall()"); // have the new module submit/exec the tx, expecting a failure event emission from both hsg and the newModule vm.expectEmit(); - emit IModuleManager.ExecutionFromModuleFailure(address(instance)); + emit IModuleManager.ExecutionFromModuleFailure(address(harness)); vm.expectEmit(); emit IAvatar.ExecutionFromModuleFailure(newModule); vm.prank(newModule); - instance.execTransactionFromModule(address(hats), 0, badCall, Enum.Operation.Call); + harness.execTransactionFromModule(address(hats), 0, badCall, Enum.Operation.Call); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_happy_delegateCall() public inSafeExecTransaction(false) inModuleExecTransaction(false) { + address target = defaultDelegatecallTargets[0]; + + uint256 expectedThreshold = safe.getThreshold(); + address expectedFallbackHandler = SafeManagerLib.getSafeFallbackHandler(safe); + bytes32 expectedOwnersHash = keccak256(abi.encode(safe.getOwners())); + + vm.prank(newModule); + harness.execTransactionFromModule(target, 0, hex"00", Enum.Operation.DelegateCall); + + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: expectedOwnersHash, + _existingThreshold: expectedThreshold, + _existingFallbackHandler: expectedFallbackHandler, + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_notModule() public { + function test_revert_notModule() public inSafeExecTransaction(false) inModuleExecTransaction(false) { uint256 preValue = address(safe).balance; uint256 transferValue = 0.3 ether; // have a non-module submit/exec the tx, expecting a revert vm.expectRevert(abi.encodeWithSelector(ModifierUnowned.NotAuthorized.selector, other)); vm.prank(other); - instance.execTransactionFromModule(recipient, transferValue, hex"00", Enum.Operation.Call); + harness.execTransactionFromModule(recipient, transferValue, hex"00", Enum.Operation.Call); // confirm the tx did not succeed by checking ETH balance changes assertEq(address(safe).balance, preValue); assertEq(recipient.balance, 0); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_moduleCannotCallSafe() public { + function test_revert_moduleCannotCallSafe() public inSafeExecTransaction(false) inModuleExecTransaction(false) { uint256 transferValue = 0.3 ether; // try to send to the safe, expecting a revert vm.expectRevert(IHatsSignerGate.CannotCallSafe.selector); vm.prank(newModule); - instance.execTransactionFromModule(address(safe), transferValue, hex"00", Enum.Operation.Call); + harness.execTransactionFromModule(address(safe), transferValue, hex"00", Enum.Operation.Call); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_delegatecallTargetNotEnabled() public { + function test_revert_delegatecallTargetNotEnabled() + public + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { address target = makeAddr("target"); // craft a delegatecall to a non-enabled target @@ -85,11 +159,69 @@ contract ExecutingFromModuleViaHSG is WithHSGInstanceTest { vm.expectRevert(IHatsSignerGate.DelegatecallTargetNotEnabled.selector); vm.prank(newModule); - instance.execTransactionFromModule(target, 0, data, Enum.Operation.DelegateCall); + harness.execTransactionFromModule(target, 0, data, Enum.Operation.DelegateCall); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_revert_inSafeExecTransaction() public inSafeExecTransaction(true) inModuleExecTransaction(false) { + address target = makeAddr("target"); + + // craft a call + bytes memory data = abi.encodeWithSignature("goodCall()"); + + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + vm.prank(newModule); + harness.execTransactionFromModule(target, 0, data, Enum.Operation.Call); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_revert_inModuleExecTransaction() public inSafeExecTransaction(false) inModuleExecTransaction(true) { + address target = makeAddr("target"); + + // craft a delegatecall to a non-enabled target + bytes memory data = abi.encodeWithSignature("goodCall()"); + + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + vm.prank(newModule); + harness.execTransactionFromModule(target, 0, data, Enum.Operation.Call); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation.Call, + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } } -contract ExecutingFromModuleReturnDataViaHSG is WithHSGInstanceTest { +contract ExecutingFromModuleReturnDataViaHSG is WithHSGHarnessInstanceTest { address newModule = tstModule1; address recipient = makeAddr("recipient"); @@ -98,66 +230,138 @@ contract ExecutingFromModuleReturnDataViaHSG is WithHSGInstanceTest { // enable a new module vm.prank(owner); - instance.enableModule(newModule); + harness.enableModule(newModule); // deal the safe some eth deal(address(safe), 1 ether); } - function test_happy_executionSuccess() public { + function test_happy_executionSuccess() public inSafeExecTransaction(false) inModuleExecTransaction(false) { uint256 preValue = address(safe).balance; uint256 transferValue = 0.3 ether; uint256 postValue = preValue - transferValue; // have the new module submit/exec the tx, expecting a success event emission from both hsg and the newModule vm.expectEmit(); - emit IModuleManager.ExecutionFromModuleSuccess(address(instance)); + emit IModuleManager.ExecutionFromModuleSuccess(address(harness)); vm.expectEmit(); emit IAvatar.ExecutionFromModuleSuccess(newModule); vm.prank(newModule); - instance.execTransactionFromModuleReturnData(recipient, transferValue, hex"00", Enum.Operation.Call); + harness.execTransactionFromModuleReturnData(recipient, transferValue, hex"00", Enum.Operation.Call); // confirm the tx succeeded by checking ETH balance changes assertEq(address(safe).balance, postValue); assertEq(recipient.balance, transferValue); + + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_happy_executionFailure() public { + function test_happy_executionFailure() public inSafeExecTransaction(false) inModuleExecTransaction(false) { // craft a call to a function that doesn't exist on a contract (we'll use Hats.sol) bytes memory badCall = abi.encodeWithSignature("badCall()"); // have the new module submit/exec the tx, expecting a failure event emission from both hsg and the newModule vm.expectEmit(); - emit IModuleManager.ExecutionFromModuleFailure(address(instance)); + emit IModuleManager.ExecutionFromModuleFailure(address(harness)); vm.expectEmit(); emit IAvatar.ExecutionFromModuleFailure(newModule); vm.prank(newModule); - instance.execTransactionFromModuleReturnData(address(hats), 0, badCall, Enum.Operation.Call); + harness.execTransactionFromModuleReturnData(address(hats), 0, badCall, Enum.Operation.Call); + + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_happy_delegateCall() public inSafeExecTransaction(false) inModuleExecTransaction(false) { + address target = defaultDelegatecallTargets[0]; + + uint256 expectedThreshold = safe.getThreshold(); + address expectedFallbackHandler = SafeManagerLib.getSafeFallbackHandler(safe); + bytes32 expectedOwnersHash = keccak256(abi.encode(safe.getOwners())); + + vm.prank(newModule); + harness.execTransactionFromModuleReturnData(target, 0, hex"00", Enum.Operation.DelegateCall); + + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: expectedOwnersHash, + _existingThreshold: expectedThreshold, + _existingFallbackHandler: expectedFallbackHandler, + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_notModule() public { + function test_revert_notModule() public inSafeExecTransaction(false) inModuleExecTransaction(false) { uint256 preValue = address(safe).balance; uint256 transferValue = 0.3 ether; // have a non-module submit/exec the tx, expecting a revert vm.expectRevert(abi.encodeWithSelector(ModifierUnowned.NotAuthorized.selector, other)); vm.prank(other); - instance.execTransactionFromModuleReturnData(recipient, transferValue, hex"00", Enum.Operation.Call); + harness.execTransactionFromModuleReturnData(recipient, transferValue, hex"00", Enum.Operation.Call); // confirm the tx did not succeed by checking ETH balance changes assertEq(address(safe).balance, preValue); assertEq(recipient.balance, 0); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_moduleCannotCallSafe() public { + function test_revert_moduleCannotCallSafe() public inSafeExecTransaction(false) inModuleExecTransaction(false) { uint256 transferValue = 0.3 ether; // try to send to the safe, expecting a revert vm.expectRevert(IHatsSignerGate.CannotCallSafe.selector); vm.prank(newModule); - instance.execTransactionFromModuleReturnData(address(safe), transferValue, hex"00", Enum.Operation.Call); + harness.execTransactionFromModuleReturnData(address(safe), transferValue, hex"00", Enum.Operation.Call); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_delegatecallTargetNotEnabled() public { + function test_revert_delegatecallTargetNotEnabled() + public + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { address target = makeAddr("target"); // craft a delegatecall to a non-enabled target @@ -165,7 +369,73 @@ contract ExecutingFromModuleReturnDataViaHSG is WithHSGInstanceTest { vm.expectRevert(IHatsSignerGate.DelegatecallTargetNotEnabled.selector); vm.prank(newModule); - instance.execTransactionFromModuleReturnData(target, 0, data, Enum.Operation.DelegateCall); + harness.execTransactionFromModuleReturnData(target, 0, data, Enum.Operation.DelegateCall); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_revert_inSafeExecTransaction(bool _inModuleExecTransaction) + public + inSafeExecTransaction(true) + inModuleExecTransaction(_inModuleExecTransaction) + { + address target = makeAddr("target"); + + // craft a call + bytes memory data = abi.encodeWithSignature("goodCall()"); + + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + vm.prank(newModule); + harness.execTransactionFromModuleReturnData(target, 0, data, Enum.Operation.Call); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_revert_inModuleExecTransaction(bool _inSafeExecTransaction) + public + inSafeExecTransaction(_inSafeExecTransaction) + inModuleExecTransaction(true) + { + address target = makeAddr("target"); + + // craft a delegatecall to a non-enabled target + bytes memory data = abi.encodeWithSignature("goodCall()"); + + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + vm.prank(newModule); + harness.execTransactionFromModuleReturnData(target, 0, data, Enum.Operation.Call); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } } diff --git a/test/HatsSignerGate.signerTxs.sol b/test/HatsSignerGate.signerTxs.sol index 12b1aac..f98c3b1 100644 --- a/test/HatsSignerGate.signerTxs.sol +++ b/test/HatsSignerGate.signerTxs.sol @@ -10,6 +10,9 @@ import { MultiSend } from "../lib/safe-smart-account/contracts/libraries/MultiSe contract ExecutingTransactions is WithHSGInstanceTest { event ExecutionSuccess(bytes32 indexed txHash, uint256 payment); + address payable recipient1 = payable(makeAddr("recipient1")); + address payable recipient2 = payable(makeAddr("recipient2")); + function testExecTxByHatWearers() public { _addSignersSameHat(3, signerHat); @@ -309,6 +312,152 @@ contract ExecutingTransactions is WithHSGInstanceTest { ); } } + + function test_happy_multiSend() public { + uint256 firstSendAmount = 0.1 ether; + uint256 secondSendAmount = 0.2 ether; + + // add 3 signers + _addSignersSameHat(3, signerHat); + + // deal the safe some ETH + deal(address(safe), 1 ether); + + // craft a multisend action to send eth twice + bytes memory packedCalls = abi.encodePacked( + // 1) first send + uint8(0), // 0 for call; 1 for delegatecall + address(recipient1), // to + uint256(firstSendAmount), // value + uint256(0), // data length + hex"", // data + // 2) second send + uint8(0), // 0 for call; 1 for delegatecall + address(recipient2), // to + uint256(secondSendAmount), // value + uint256(0), // data length + hex"" // data + ); + + bytes memory multiSendData = abi.encodeWithSignature("multiSend(bytes)", packedCalls); + + // get the tx hash + bytes32 safeTxHash = safe.getTransactionHash( + defaultDelegatecallTargets[0], + 0, // value + multiSendData, // data + Enum.Operation.DelegateCall, // operation + 0, // safeTxGas + 0, // baseGas + 0, // gasPrice + address(0), // gasToken + payable(address(0)), // refundReceiver + safe.nonce() // nonce + ); + + // sufficient signers sign it + bytes memory sigs = _createNSigsForTx(safeTxHash, 2); + + // execute the tx + safe.execTransaction( + defaultDelegatecallTargets[0], + 0, + multiSendData, + Enum.Operation.DelegateCall, + 0, + 0, + 0, + address(0), + payable(address(0)), + sigs + ); + + // confirm correct balances + assertEq(recipient1.balance, firstSendAmount, "wrong recipient1 balance"); + assertEq(recipient2.balance, secondSendAmount, "wrong recipient2 balance"); + } + + function test_happy_batchMultiSend(uint256 _batchSize) public { + // construct an N-action multisend that will call execTransaction a random number of times + uint256 batchSize = bound(_batchSize, 1, 50); + // ensure the safe has enough ETH to cover the batch + deal(address(safe), batchSize * 1 ether); + + // add 3 signers + _addSignersSameHat(3, signerHat); + + uint256 sendAmount = 0.1 ether; + bytes32[] memory txHashes = new bytes32[](batchSize); + bytes[] memory signatures = new bytes[](batchSize); + bytes[] memory actionData = new bytes[](batchSize); + bytes memory packedCalls; + + uint256 startingNonce = safe.nonce(); + + for (uint256 i; i < batchSize; i++) { + // get the tx hash for each action + txHashes[i] = safe.getTransactionHash( + recipient1, + sendAmount, // value + hex"", // data + Enum.Operation.Call, // operation + 0, // safeTxGas + 0, // baseGas + 0, // gasPrice + address(0), // gasToken + payable(address(0)), // refundReceiver + startingNonce + i // nonce needs to increment for each tx + ); + + // sufficient signers sign each action + signatures[i] = _createNSigsForTx(txHashes[i], 2); + + // encode each Safe.execTransaction call + actionData[i] = abi.encodeWithSignature( + "execTransaction(address,uint256,bytes,uint8,uint256,uint256,uint256,address,address,bytes)", + recipient1, + sendAmount, // value + hex"", + Enum.Operation.Call, + 0, + 0, + 0, + address(0), + payable(address(0)), + signatures[i] + ); + + // encode the action data for multiSend + bytes memory packedCall = abi.encodePacked( + uint8(0), // 0 for call; 1 for delegatecall + address(safe), // to + uint256(0), // value + uint256(actionData[i].length), // data length + actionData[i] // data + ); + + // append the action data into a multisend call + packedCalls = abi.encodePacked(packedCalls, packedCall); + } + + // execute the multisend + MultiSend(defaultDelegatecallTargets[0]).multiSend(packedCalls); + + // confirm correct balances + assertEq(recipient1.balance, sendAmount * batchSize, "wrong recipient1 balance"); + } + + function test_happy_multiSend2() public { + test_happy_batchMultiSend(2); + } + + function test_happy_multiSend10() public { + test_happy_batchMultiSend(10); + } + + function test_happy_multiSend50() public { + test_happy_batchMultiSend(50); + } } contract ConstrainingSigners is WithHSGInstanceTest { @@ -664,9 +813,10 @@ contract HSGGuarding is WithHSGInstanceTest { // have 3 signers sign it bytes memory signatures = _createNSigsForTx(txHash, signerCount); - // we expect the `sender` param to be address(0) because the sender param from hsg.checkTransaction is empty + // we expect the `sender` param to be the Safe address because the sender param from hsg.checkTransaction is the + // Safe address vm.expectEmit(); - emit TestGuard.PreChecked(address(0)); + emit TestGuard.PreChecked(address(safe)); vm.expectEmit(); emit TestGuard.PostChecked(true); diff --git a/test/HatsSignerGate.t.sol b/test/HatsSignerGate.t.sol index adcf96f..7f3a4a0 100644 --- a/test/HatsSignerGate.t.sol +++ b/test/HatsSignerGate.t.sol @@ -152,7 +152,7 @@ contract InstanceDeployment is TestSuite { hsgModules: tstModules }); bytes memory initializeParams = abi.encode(setupParams); - console2.logBytes(initializeParams); + // console2.logBytes(initializeParams); vm.expectRevert(InvalidInitialization.selector); instance.setUp(initializeParams); } @@ -1240,17 +1240,27 @@ contract SettingGuard is WithHSGInstanceTest { /// tests call harness.exposed_checkTransaction, which wraps instance.checkTransaction and stores the transient state in /// persistent storage for access in tests. contract CheckTransaction is WithHSGHarnessInstanceTest { + uint256 public simulatedInitialNonce; + function setUp() public override { super.setUp(); - // execute an empty transaction from the safe to set the nonce to 1 to avoid underflows when we check the nonce in - // checkTransaction. This also adds two signers. + // Execute an empty transaction from the safe to set the nonce to force the safe's nonce to increment. This is + // necessary to simulate the conditions under which HSG.checkTransaction is called in practice, ie just after the + // nonce has incremented. This also adds two signers. _executeEmptyCallFromSafe(2, address(org)); - assertEq(safe.nonce(), 1, "safe nonce should be 1"); + assertGt(safe.nonce(), 0, "safe nonce should gt 0"); + + simulatedInitialNonce = safe.nonce() - 1; } - function test_happy_checkTransaction_callToNonSafe(uint256 _seed) public callerIsSafe(true) { + function test_happy_checkTransaction_callToNonSafe(uint256 _seed) + public + callerIsSafe(true) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { address target = _getRandomAddress(_seed); vm.assume(target != address(safe)); @@ -1264,20 +1274,49 @@ contract CheckTransaction is WithHSGHarnessInstanceTest { target, 0, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(0), signatures, address(0) ); - _assertTransientStateVariables(Enum.Operation.Call, bytes32(0), 0, address(0), 0, safe.nonce() - 1, 1); + _assertTransientStateVariables({ + _operation: Enum.Operation.Call, // this is a call + _existingOwnersHash: bytes32(0), // empty because call + _existingThreshold: 0, // empty because call + _existingFallbackHandler: address(0), // empty because call + _inSafeExecTransaction: true, + _inModuleExecTransaction: false, // not a module tx + _initialNonce: simulatedInitialNonce, + _checkTransactionCounter: 1 + }); } - function test_revert_notCalledFromSafe() public callerIsSafe(false) { + function test_revert_notCalledFromSafe() + public + callerIsSafe(false) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { vm.expectRevert(IHatsSignerGate.NotCalledFromSafe.selector); vm.prank(caller); harness.exposed_checkTransaction( address(0), 0, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(0), new bytes(0), address(0) ); - _assertTransientStateVariables(Enum.Operation(uint8(0)), bytes32(0), 0, address(0), 0, 0, 0); + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_guardReverts() public callerIsSafe(true) { + function test_revert_guardReverts() + public + callerIsSafe(true) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { // add a test guard vm.prank(owner); harness.setGuard(address(tstGuard)); @@ -1292,10 +1331,25 @@ contract CheckTransaction is WithHSGHarnessInstanceTest { address(0), 0, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(0), new bytes(0), address(0) ); - _assertTransientStateVariables(Enum.Operation(uint8(0)), bytes32(0), 0, address(0), 0, 0, 0); + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_delegatecallTargetEnabled() public callerIsSafe(true) { + function test_delegatecallTargetEnabled() + public + callerIsSafe(true) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { // enable a delegatecall target address target = defaultDelegatecallTargets[0]; vm.prank(owner); @@ -1315,18 +1369,25 @@ contract CheckTransaction is WithHSGHarnessInstanceTest { target, 0, new bytes(0), Enum.Operation.DelegateCall, 0, 0, 0, address(0), payable(0), signatures, address(0) ); - _assertTransientStateVariables( - Enum.Operation.DelegateCall, - expectedOwnersHash, - expectedThreshold, - expectedFallbackHandler, - 0, - safe.nonce() - 1, - 1 - ); + // transient state should be populated + _assertTransientStateVariables({ + _operation: Enum.Operation.DelegateCall, // delegatecall + _existingOwnersHash: expectedOwnersHash, // populated since delegatecall + _existingThreshold: expectedThreshold, // populated since delegatecall + _existingFallbackHandler: expectedFallbackHandler, // populated since delegatecall + _inSafeExecTransaction: true, + _inModuleExecTransaction: false, // not a module tx + _initialNonce: simulatedInitialNonce, + _checkTransactionCounter: 1 + }); } - function test_revert_delegatecallTargetNotEnabled() public callerIsSafe(true) { + function test_revert_delegatecallTargetNotEnabled() + public + callerIsSafe(true) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { // expect the checkTransaction to revert vm.expectRevert(IHatsSignerGate.DelegatecallTargetNotEnabled.selector); vm.prank(caller); @@ -1344,20 +1405,50 @@ contract CheckTransaction is WithHSGHarnessInstanceTest { address(0) ); - _assertTransientStateVariables(Enum.Operation(uint8(0)), bytes32(0), 0, address(0), 0, 0, 0); + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_cannotCallSafe() public callerIsSafe(true) { + function test_revert_cannotCallSafe() + public + callerIsSafe(true) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { vm.expectRevert(IHatsSignerGate.CannotCallSafe.selector); vm.prank(caller); harness.exposed_checkTransaction( address(safe), 0, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(0), new bytes(0), address(0) ); - _assertTransientStateVariables(Enum.Operation(uint8(0)), bytes32(0), 0, address(0), 0, 0, 0); + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_thresholdTooLow(uint8 _operation) public callerIsSafe(true) { + function test_revert_thresholdTooLow(uint8 _operation) + public + callerIsSafe(true) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { // bound the arg Enum.Operation operation = Enum.Operation(bound(_operation, 0, 1)); @@ -1384,10 +1475,25 @@ contract CheckTransaction is WithHSGHarnessInstanceTest { target, 0, new bytes(0), operation, 0, 0, 0, address(0), payable(0), new bytes(0), address(0) ); - _assertTransientStateVariables(Enum.Operation(uint8(0)), bytes32(0), 0, address(0), 0, 0, 0); + // transient state should be cleared + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_insufficientValidSignatures(uint8 _operation) public callerIsSafe(true) { + function test_revert_insufficientValidSignatures(uint8 _operation) + public + callerIsSafe(true) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { // bound the arg Enum.Operation operation = Enum.Operation(bound(_operation, 0, 1)); @@ -1415,10 +1521,75 @@ contract CheckTransaction is WithHSGHarnessInstanceTest { target, 0, new bytes(0), operation, 0, 0, 0, address(0), payable(0), signatures, address(0) ); - _assertTransientStateVariables(Enum.Operation(uint8(0)), bytes32(0), 0, address(0), 0, 0, 0); + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_revert_inSafeExecTransaction(bool _inModuleExecTransaction) + public + callerIsSafe(true) + inSafeExecTransaction(true) + inModuleExecTransaction(_inModuleExecTransaction) + { + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + vm.prank(caller); + harness.exposed_checkTransaction( + address(safe), 0, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(0), new bytes(0), address(0) + ); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_noReentryAllowed() public callerIsSafe(true) { + function test_revert_inModuleExecTransaction(bool _inSafeExecTransaction) + public + callerIsSafe(true) + inSafeExecTransaction(_inSafeExecTransaction) + inModuleExecTransaction(true) + { + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + vm.prank(caller); + harness.exposed_checkTransaction( + address(safe), 0, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(0), new bytes(0), address(0) + ); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_revert_noReentryAllowed() + public + callerIsSafe(true) + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { // first craft a dummy/empty tx to pass to checkTransaction bytes32 dummyTxHash = safe.getTransactionHash( address(this), // send 0 eth to this contract @@ -1465,6 +1636,18 @@ contract CheckTransaction is WithHSGHarnessInstanceTest { safe.execTransaction( address(harness), 0, reentryCall, Enum.Operation.Call, 0, 0, 0, address(0), payable(0), signatures ); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } } @@ -1474,13 +1657,31 @@ contract CheckTransaction is WithHSGHarnessInstanceTest { /// Additonally, these tests do not cover the internal Safe state checks. See /// HatsSignerGate.internals.t.sol:TransactionValidationInternals for comprehensive tests of that logic. contract CheckAfterExecution is WithHSGHarnessInstanceTest { - function test_happy_checkAfterExecution(bytes32 _txHash, bool _success) public callerIsSafe(true) { + function test_happy_checkAfterExecution(bytes32 _txHash, bool _success) + public + inSafeExecTransaction(true) + inModuleExecTransaction(false) + { // call to checkAfterExecution should not revert - vm.prank(caller); harness.exposed_checkAfterExecution(_txHash, _success); + + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } - function test_revert_guardReverts(bytes32 _txHash, bool _success) public callerIsSafe(true) { + function test_revert_guardReverts(bytes32 _txHash, bool _success) + public + inSafeExecTransaction(true) + inModuleExecTransaction(false) + { // add a test guard vm.prank(owner); harness.setGuard(address(tstGuard)); @@ -1490,8 +1691,63 @@ contract CheckAfterExecution is WithHSGHarnessInstanceTest { // call to checkAfterExecution should revert vm.expectRevert(); - vm.prank(caller); harness.exposed_checkAfterExecution(_txHash, _success); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_revert_notInSafeExecTransaction(bytes32 _txHash, bool _success) + public + inSafeExecTransaction(false) + inModuleExecTransaction(false) + { + // should revert because we are not inside a Safe execTransaction call + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + harness.exposed_checkAfterExecution(_txHash, _success); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); + } + + function test_revert_inModuleExecTransaction(bytes32 _txHash, bool _success) + public + inSafeExecTransaction(true) + inModuleExecTransaction(true) + { + // should revert because we are not inside a Safe execTransaction call + vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector); + harness.exposed_checkAfterExecution(_txHash, _success); + + // transient state should be cleared after revert + _assertTransientStateVariables({ + _operation: Enum.Operation(uint8(0)), + _existingOwnersHash: bytes32(0), + _existingThreshold: 0, + _existingFallbackHandler: address(0), + _inSafeExecTransaction: false, + _inModuleExecTransaction: false, + _initialNonce: 0, + _checkTransactionCounter: 0 + }); } } @@ -1526,148 +1782,3 @@ contract Views is WithHSGInstanceTest { assertFalse(instance.canAttachToSafe(newSafe), "should not be attachable"); } } - -contract HSGGuarding is WithHSGInstanceTest { - uint256 public disallowedValue = 1337; - uint256 public goodValue = 9_000_000_000; - address public recipient = makeAddr("recipient"); - uint256 public signerCount = 2; - - function setUp() public override { - super.setUp(); - - // set it on our hsg instance - vm.prank(owner); - instance.setGuard(address(tstGuard)); - assertEq(instance.getGuard(), address(tstGuard), "guard should be tstGuard"); - - // deal the safe some eth - deal(address(safe), 1 ether); - - // add signerCount number of signers - _addSignersSameHat(signerCount, signerHat); - - address[] memory owners = safe.getOwners(); - assertEq(owners.length, signerCount, "owners should be signerCount"); - } - - /// @dev a successful transaction should hit the tstGuard's checkTransaction and checkAfterExecution funcs - function test_executed() public { - uint256 preNonce = safe.nonce(); - uint256 preValue = address(safe).balance; - uint256 transferValue = goodValue; - uint256 postValue = preValue - transferValue; - - // create the tx - bytes32 txHash = _getTxHash(recipient, transferValue, Enum.Operation.Call, hex"00", safe); - - // have 3 signers sign it - bytes memory signatures = _createNSigsForTx(txHash, signerCount); - - // we expect the `sender` param to be address(0) because the sender param from hsg.checkTransaction is empty - vm.expectEmit(); - emit TestGuard.PreChecked(address(0)); - vm.expectEmit(); - emit TestGuard.PostChecked(true); - - // have one of the signers submit/exec the tx - vm.prank(signerAddresses[0]); - safe.execTransaction( - recipient, - transferValue, - hex"00", - Enum.Operation.Call, - // not using the refunder - 0, - 0, - 0, - address(0), - payable(address(0)), - signatures - ); - - // confirm the tx succeeded by checking ETH balance changes - assertEq(address(safe).balance, postValue); - assertEq(recipient.balance, transferValue); - assertEq(safe.nonce(), preNonce + 1); - } - - // the test guard should revert in checkTransaction - function test_revert_checkTransaction() public { - // we make this happen by using a bad value in the safe.execTransaction call - uint256 preNonce = safe.nonce(); - uint256 preValue = address(safe).balance; - uint256 transferValue = disallowedValue; - - // create the tx - bytes32 txHash = _getTxHash(recipient, transferValue, Enum.Operation.Call, hex"00", safe); - - // have 3 signers sign it - bytes memory signatures = _createNSigsForTx(txHash, signerCount); - - // we expect the test guard to revert in checkTransaction - vm.expectRevert("Cannot send 1337"); - - // have one of the signers submit/exec the tx - vm.prank(signerAddresses[0]); - safe.execTransaction( - recipient, - transferValue, - hex"00", - Enum.Operation.Call, - // not using the refunder - 0, - 0, - 0, - address(0), - payable(address(0)), - signatures - ); - - // confirm the tx did not succeed by checking ETH balance changes - assertEq(address(safe).balance, preValue); - assertEq(recipient.balance, 0); - assertEq(safe.nonce(), preNonce); - } - - // the test guard should revert in checkAfterExecution - function test_revert_checkAfterExecution() public { - // we make this happen by setting the test guard to disallow execution - tstGuard.disallowExecution(); - - // craft a basic eth transfer tx - uint256 preNonce = safe.nonce(); - uint256 preValue = address(safe).balance; - uint256 transferValue = goodValue; - - // create the tx - bytes32 txHash = _getTxHash(recipient, transferValue, Enum.Operation.Call, hex"00", safe); - - // have 3 signers sign it - bytes memory signatures = _createNSigsForTx(txHash, signerCount); - - // we expect the test guard to revert in checkTransaction - vm.expectRevert("Reverted in checkAfterExecution"); - - // have one of the signers submit/exec the tx - vm.prank(signerAddresses[0]); - safe.execTransaction( - recipient, - transferValue, - hex"00", - Enum.Operation.Call, - // not using the refunder - 0, - 0, - 0, - address(0), - payable(address(0)), - signatures - ); - - // confirm the tx did not succeed by checking ETH balance changes - assertEq(address(safe).balance, preValue); - assertEq(recipient.balance, 0); - assertEq(safe.nonce(), preNonce); - } -} diff --git a/test/TestSuite.t.sol b/test/TestSuite.t.sol index ded3a71..6e709bf 100644 --- a/test/TestSuite.t.sol +++ b/test/TestSuite.t.sol @@ -1022,22 +1022,26 @@ contract WithHSGHarnessInstanceTest is TestSuite { bytes32 _existingOwnersHash, uint256 _existingThreshold, address _existingFallbackHandler, - uint256 _reentrancyGuard, + bool _inSafeExecTransaction, + bool _inModuleExecTransaction, uint256 _initialNonce, - uint256 _entrancyCounter + uint256 _checkTransactionCounter ) internal view { - assertEq(uint8(harness.operation()), uint8(_operation), "operation should be set"); - assertEq(harness.reentrancyGuard(), _reentrancyGuard, "reentrancy guard should be set"); - assertEq(harness.initialNonce(), _initialNonce, "initial nonce should be set"); - assertEq(harness.entrancyCounter(), _entrancyCounter, "entrancy counter should be set"); - if (_operation == Enum.Operation.DelegateCall) { - assertEq(harness.existingOwnersHash(), _existingOwnersHash, "existing owners hash should be set"); - assertEq(harness.existingThreshold(), _existingThreshold, "existing threshold should be set"); - assertEq(harness.existingFallbackHandler(), _existingFallbackHandler, "existing fallback handler should be set"); - } else { - assertEq(harness.existingOwnersHash(), bytes32(0), "existing owners hash should be empty"); - assertEq(harness.existingThreshold(), 0, "existing threshold should be empty"); - assertEq(harness.existingFallbackHandler(), address(0), "existing fallback handler should be empty"); + { + assertEq(uint8(harness.operation()), uint8(_operation), "operation should be set"); + assertEq(harness.inSafeExecTransaction(), _inSafeExecTransaction, "inSafeExecTransaction should be set"); + assertEq(harness.inModuleExecTransaction(), _inModuleExecTransaction, "inModuleExecTransaction should be set"); + assertEq(harness.initialNonce(), _initialNonce, "initial nonce should be set"); + assertEq(harness.checkTransactionCounter(), _checkTransactionCounter, "checkTransactionCounter should be set"); + if (_operation == Enum.Operation.DelegateCall) { + assertEq(harness.existingOwnersHash(), _existingOwnersHash, "existing owners hash should be set"); + assertEq(harness.existingThreshold(), _existingThreshold, "existing threshold should be set"); + assertEq(harness.existingFallbackHandler(), _existingFallbackHandler, "existing fallback handler should be set"); + } else { + assertEq(harness.existingOwnersHash(), bytes32(0), "existing owners hash should be empty"); + assertEq(harness.existingThreshold(), 0, "existing threshold should be empty"); + assertEq(harness.existingFallbackHandler(), address(0), "existing fallback handler should be empty"); + } } } @@ -1056,4 +1060,16 @@ contract WithHSGHarnessInstanceTest is TestSuite { "the existing fallback handler should be unchanged" ); } + + /// @dev Forces the value of the `_inSafeExecTransaction` transient variable + modifier inSafeExecTransaction(bool _inSafeExecTransaction) { + harness.setInSafeExecTransaction(_inSafeExecTransaction); + _; + } + + /// @dev Forces the value of the `_inModuleExecTransaction` transient variable + modifier inModuleExecTransaction(bool _inModuleExecTransaction) { + harness.setInModuleExecTransaction(_inModuleExecTransaction); + _; + } } diff --git a/test/harnesses/HatsSignerGateHarness.sol b/test/harnesses/HatsSignerGateHarness.sol index 5b5b8d4..d5134f3 100644 --- a/test/harnesses/HatsSignerGateHarness.sol +++ b/test/harnesses/HatsSignerGateHarness.sol @@ -31,9 +31,10 @@ contract HatsSignerGateHarness is HatsSignerGate, SafeManagerLibHarness { uint256 public existingThreshold; address public existingFallbackHandler; Enum.Operation public operation; - uint256 public reentrancyGuard; + bool public inModuleExecTransaction; + bool public inSafeExecTransaction; uint256 public initialNonce; - uint256 public entrancyCounter; + uint256 public checkTransactionCounter; /*////////////////////////////////////////////////////////////// TRANSIENT STATE SETTERS @@ -51,6 +52,14 @@ contract HatsSignerGateHarness is HatsSignerGate, SafeManagerLibHarness { _existingFallbackHandler = existingFallbackHandler_; } + function setInModuleExecTransaction(bool inModuleExecTransaction_) public { + _inModuleExecTransaction = inModuleExecTransaction_; + } + + function setInSafeExecTransaction(bool inSafeExecTransaction_) public { + _inSafeExecTransaction = inSafeExecTransaction_; + } + /*////////////////////////////////////////////////////////////// EXPOSED INTERNAL FUNCTIONS //////////////////////////////////////////////////////////////*/ @@ -151,16 +160,20 @@ contract HatsSignerGateHarness is HatsSignerGate, SafeManagerLibHarness { return _operation; } - function exposed_reentrancyGuard() public view returns (uint256) { - return _reentrancyGuard; + function exposed_inModuleExecTransaction() public view returns (bool) { + return _inModuleExecTransaction; + } + + function exposed_inSafeExecTransaction() public view returns (bool) { + return _inSafeExecTransaction; } function exposed_initialNonce() public view returns (uint256) { return _initialNonce; } - function exposed_entrancyCounter() public view returns (uint256) { - return _entrancyCounter; + function exposed_checkTransactionCounter() public view returns (uint256) { + return _checkTransactionCounter; } /// @dev Exposes the transient state variables set within checkTransaction @@ -184,9 +197,10 @@ contract HatsSignerGateHarness is HatsSignerGate, SafeManagerLibHarness { existingOwnersHash = _existingOwnersHash; existingThreshold = _existingThreshold; existingFallbackHandler = _existingFallbackHandler; - reentrancyGuard = _reentrancyGuard; + inModuleExecTransaction = _inModuleExecTransaction; + inSafeExecTransaction = _inSafeExecTransaction; initialNonce = _initialNonce; - entrancyCounter = _entrancyCounter; + checkTransactionCounter = _checkTransactionCounter; } /// @dev Allows tests to call checkAfterExecution by mocking the guardEntries transient state variable diff --git a/test/mocks/TestGuard.sol b/test/mocks/TestGuard.sol index ff9c3a8..6302605 100644 --- a/test/mocks/TestGuard.sol +++ b/test/mocks/TestGuard.sol @@ -31,11 +31,12 @@ contract TestGuard is BaseGuard { executionDisallowed = true; } + /// @dev Modified to remove the operation restriction function checkTransaction( address to, uint256 value, bytes memory data, - Enum.Operation operation, + Enum.Operation, /* operation */ uint256, uint256, uint256, @@ -47,7 +48,7 @@ contract TestGuard is BaseGuard { require(to != address(0), "Cannot send to zero address"); require(value != 1337, "Cannot send 1337"); require(bytes3(data) != bytes3(0xbaddad), "Cannot call 0xbaddad"); - require(operation != Enum.Operation(1), "No delegate calls"); + // require(operation != Enum.Operation(1), "No delegate calls"); emit PreChecked(sender); }