Merge pull request #4 from sense-finance/dev

feat: make compatibility fb handler sig validation upgradable

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();

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.

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 ----
 }

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);
     }