From e9f5c9627d50acb7e9cb0787df87e96cef8fbb0b Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Sat, 24 Aug 2024 21:03:13 -0700 Subject: [PATCH] feat: make compatibility fb handler sig validation upgradable --- script/RumpelWalletFactory.s.sol | 6 ++- src/RumpelGuard.sol | 1 - src/external/CompatibilityFallbackHandler.sol | 46 +++++++++++++++++-- test/RumpelWallet.t.sol | 4 +- 4 files changed, 47 insertions(+), 10 deletions(-) diff --git a/script/RumpelWalletFactory.s.sol b/script/RumpelWalletFactory.s.sol index 578197b..7c28c91 100644 --- a/script/RumpelWalletFactory.s.sol +++ b/script/RumpelWalletFactory.s.sol @@ -8,7 +8,7 @@ import {RumpelWalletFactory} from "../src/RumpelWalletFactory.sol"; import {RumpelModule} from "../src/RumpelModule.sol"; import {InitializationScript} from "../src/InitializationScript.sol"; import {RumpelGuard} from "../src/RumpelGuard.sol"; -import {CompatibilityFallbackHandler} from "../src/external/CompatibilityFallbackHandler.sol"; +import {CompatibilityFallbackHandler, ValidationBeacon} from "../src/external/CompatibilityFallbackHandler.sol"; import {ISafeProxyFactory} from "../src/interfaces/external/ISafeProxyFactory.sol"; import {ISafe} from "../src/interfaces/external/ISafe.sol"; @@ -35,7 +35,9 @@ contract RumpelWalletFactoryScripts is Script { RumpelGuard rumpelGuard = new RumpelGuard(MAINNET_SIGN_MESSAGE_LIB); - CompatibilityFallbackHandler compatibilityFallbackHandler = new CompatibilityFallbackHandler(); + ValidationBeacon validationBeacon = new ValidationBeacon(); + CompatibilityFallbackHandler compatibilityFallbackHandler = + new CompatibilityFallbackHandler(msg.sender, validationBeacon); // Script that will be delegatecalled to enable modules InitializationScript initializationScript = new InitializationScript(); diff --git a/src/RumpelGuard.sol b/src/RumpelGuard.sol index 1bab8dd..dd75221 100644 --- a/src/RumpelGuard.sol +++ b/src/RumpelGuard.sol @@ -5,7 +5,6 @@ import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {Enum} from "./interfaces/external/ISafe.sol"; import {IGuard} from "./interfaces/external/IGuard.sol"; -import {console} from "forge-std/console.sol"; /// @notice Rumpel Safe Guard with a blocklist for the Rumpel Wallet. /// @dev Compatible with Safe v1.3.0-libs.0, the last Safe Ethereum mainnet release, so it can't use module execution hooks. diff --git a/src/external/CompatibilityFallbackHandler.sol b/src/external/CompatibilityFallbackHandler.sol index e3bc0aa..9c96b07 100644 --- a/src/external/CompatibilityFallbackHandler.sol +++ b/src/external/CompatibilityFallbackHandler.sol @@ -5,10 +5,27 @@ import "safe-smart-account/contracts/handler/TokenCallbackHandler.sol"; import "safe-smart-account/contracts/Safe.sol"; import "safe-smart-account/contracts/interfaces/ISignatureValidator.sol"; +interface IValidationBeacon { + function isValidSignature(bytes calldata messageData, bytes32 messageHash, bytes calldata signature, Safe safe) + external + view + returns (bool); +} + +contract ValidationBeacon is IValidationBeacon { + function isValidSignature(bytes calldata messageData, bytes32 messageHash, bytes calldata signature, Safe safe) + external + view + returns (bool) + { + return safe.signedMessages(messageHash) != 0; + } +} + /** * @dev IMPORTANT ---- * This contract is an exact copy of the CompatibilityFallbackHandler from the safe-smart-account repo at Tag v1.4.1, - * except with the checkSignatures path for EIP-1271 signature validation removed. + * except with the checkSignatures path for EIP-1271 signature validation removed and replaced with a call to the validation beacon. * This is to prevent the attack mentioned here: https://github.com/sense-finance/point-tokenization-vault/issues/28 * @dev IMPORTANT ---- */ @@ -26,6 +43,18 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat address internal constant SENTINEL_MODULES = address(0x1); bytes4 internal constant UPDATED_MAGIC_VALUE = 0x1626ba7e; + /// @dev THIS IS THE CHANGE ---- + + address public owner; + IValidationBeacon public validationBeacon; + + constructor(address _owner, IValidationBeacon _validationBeacon) { + owner = _owner; + validationBeacon = _validationBeacon; + } + + /// @dev THIS IS THE CHANGE ---- + /** * @notice Legacy EIP-1271 signature validation method. * @dev Implementation of ISignatureValidator (see `interfaces/ISignatureValidator.sol`) @@ -42,8 +71,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat /// @dev THIS IS THE CHANGE ---- - // Always check storage for signed messages; never checkSignatures - require(safe.signedMessages(messageHash) != 0, "Hash not approved"); + require(validationBeacon.isValidSignature(messageData, messageHash, _signature, safe), "Invalid signature"); /// @dev THIS IS THE CHANGE ---- @@ -94,8 +122,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat /// @dev THIS IS THE CHANGE ---- - // Always check storage for signed messages; never checkSignatures - require(safe.signedMessages(messageHash) != 0, "Hash not approved"); + require(validationBeacon.isValidSignature(messageData, messageHash, _signature, safe), "Invalid signature"); /// @dev THIS IS THE CHANGE ---- @@ -190,4 +217,13 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat if iszero(mload(0x00)) { revert(add(response, 0x20), mload(response)) } } } + + /// @dev THIS IS THE CHANGE ---- + + function setValidationBeacon(IValidationBeacon _validationBeacon) external { + require(msg.sender == owner, "only owner"); + validationBeacon = _validationBeacon; + } + + /// @dev THIS IS THE CHANGE ---- } diff --git a/test/RumpelWallet.t.sol b/test/RumpelWallet.t.sol index 9adfab6..9a8b684 100644 --- a/test/RumpelWallet.t.sol +++ b/test/RumpelWallet.t.sol @@ -276,7 +276,7 @@ contract RumpelWalletTest is Test { assertEq(safe.isValidSignature(keccak256(message), emptySignature), EIP1271_MAGIC_VALUE); // Demonstrate that an incorrect message hash fails - vm.expectRevert("Hash not approved"); + vm.expectRevert("Invalid signature"); bytes memory incorrectMessage = "Hello Safe bad"; safe.isValidSignature(incorrectMessage, emptySignature); } @@ -590,7 +590,7 @@ contract RumpelWalletTest is Test { assertEq(safe.isValidSignature(keccak256(message), "0x1234"), EIP1271_MAGIC_VALUE); // Demonstrate that an incorrect message hash fails - vm.expectRevert("Hash not approved"); + vm.expectRevert("Invalid signature"); bytes memory incorrectMessage = "Hello Safe bad"; safe.isValidSignature(incorrectMessage, emptySignature); }