Skip to content

Commit

Permalink
fix: remove legacy checkSignatures path from isValidSignature compati…
Browse files Browse the repository at this point in the history
…bility handler
  • Loading branch information
jparklev committed Aug 19, 2024
1 parent 49ae608 commit 0611c27
Show file tree
Hide file tree
Showing 6 changed files with 195 additions and 3 deletions.
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@
[submodule "lib/point-tokenization-vault"]
path = lib/point-tokenization-vault
url = https://github.com/sense-finance/point-tokenization-vault
[submodule "safe-smart-account"]
path = safe-smart-account
url = https://github.com/safe-global/safe-smart-account/
3 changes: 2 additions & 1 deletion remappings.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
@openzeppelin/=lib/openzeppelin-contracts/
solmate/=lib/solmate/src/
forge-std/=lib/forge-std/src/
forge-std/=lib/forge-std/src/
safe-smart-account/=safe-smart-account/
1 change: 1 addition & 0 deletions safe-smart-account
Submodule safe-smart-account added at bf943f
6 changes: 4 additions & 2 deletions script/RumpelWalletFactory.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +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 {ISafeProxyFactory} from "../src/interfaces/external/ISafeProxyFactory.sol";
import {ISafe} from "../src/interfaces/external/ISafe.sol";

Expand All @@ -16,7 +17,6 @@ contract RumpelWalletFactoryScripts is Script {
address public MAINNET_RUMPEL_VAULT = 0x1EeEBa76f211C4Dce994b9c5A74BDF25DB649Fa1;
address public MAINNET_SAFE_SINGLETON = 0xd9Db270c1B5E3Bd161E8c8503c55cEABeE709552;
ISafeProxyFactory public MAINNET_SAFE_PROXY_FACTORY = ISafeProxyFactory(0xa6B71E26C5e0845f74c812102Ca7114b6a896AB2);
address public MAINNET_COMPATIBILITY_FALLBACK = 0xf48f2B2d2a534e402487b3ee7C18c33Aec0Fe5e4;

address public MAINNET_SIGN_MESSAGE_LIB = 0xA65387F16B013cf2Af4605Ad8aA5ec25a2cbA3a2;

Expand All @@ -35,12 +35,14 @@ contract RumpelWalletFactoryScripts is Script {

RumpelGuard rumpelGuard = new RumpelGuard(MAINNET_SIGN_MESSAGE_LIB);

CompatibilityFallbackHandler compatibilityFallbackHandler = new CompatibilityFallbackHandler();

// Script that will be delegatecalled to enable modules
InitializationScript initializationScript = new InitializationScript();

RumpelWalletFactory rumpelWalletFactory = new RumpelWalletFactory(
MAINNET_SAFE_PROXY_FACTORY,
MAINNET_COMPATIBILITY_FALLBACK,
address(compatibilityFallbackHandler),
MAINNET_SAFE_SINGLETON,
address(rumpelModule),
address(rumpelGuard),
Expand Down
182 changes: 182 additions & 0 deletions src/external/CompatibilityFallbackHandler.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;

import "safe-smart-account/contracts/handler/TokenCallbackHandler.sol";
import "safe-smart-account/contracts/Safe.sol";
import "safe-smart-account/contracts/interfaces/ISignatureValidator.sol";

/**
* @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 legacy EIP-1271 signature validation removed.
* This is to prevent the attack mentioned here: https://github.com/sense-finance/point-tokenization-vault/issues/28
* @dev IMPORTANT ----
*/

/**
* @title Compatibility Fallback Handler - Provides compatibility between pre 1.3.0 and 1.3.0+ Safe contracts.
* @author Richard Meissner - @rmeissner
*/
contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidator {
// keccak256("SafeMessage(bytes message)");
bytes32 private constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca;

bytes4 internal constant SIMULATE_SELECTOR = bytes4(keccak256("simulate(address,bytes)"));

address internal constant SENTINEL_MODULES = address(0x1);
bytes4 internal constant UPDATED_MAGIC_VALUE = 0x1626ba7e;

/**
* @notice Legacy EIP-1271 signature validation method.
* @dev Implementation of ISignatureValidator (see `interfaces/ISignatureValidator.sol`)
* @param _data Arbitrary length data signed on the behalf of address(msg.sender).
* @param _signature Signature byte array associated with _data.
* @return The EIP-1271 magic value.
*/
function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
// Caller should be a Safe
Safe safe = Safe(payable(msg.sender));
bytes memory messageData = encodeMessageDataForSafe(safe, _data);
bytes32 messageHash = keccak256(messageData);

/// @dev THIS IS THE CHANGE ----

// Always check storage for signed messages; never checkSignatures
require(safe.signedMessages(messageHash) != 0, "Hash not approved");

/// @dev THIS IS THE CHANGE ----

return EIP1271_MAGIC_VALUE;
}

/**
* @dev Returns the hash of a message to be signed by owners.
* @param message Raw message bytes.
* @return Message hash.
*/
function getMessageHash(bytes memory message) public view returns (bytes32) {
return getMessageHashForSafe(Safe(payable(msg.sender)), message);
}

/**
* @dev Returns the pre-image of the message hash (see getMessageHashForSafe).
* @param safe Safe to which the message is targeted.
* @param message Message that should be encoded.
* @return Encoded message.
*/
function encodeMessageDataForSafe(Safe safe, bytes memory message) public view returns (bytes memory) {
bytes32 safeMessageHash = keccak256(abi.encode(SAFE_MSG_TYPEHASH, keccak256(message)));
return abi.encodePacked(bytes1(0x19), bytes1(0x01), safe.domainSeparator(), safeMessageHash);
}

/**
* @dev Returns hash of a message that can be signed by owners.
* @param safe Safe to which the message is targeted.
* @param message Message that should be hashed.
* @return Message hash.
*/
function getMessageHashForSafe(Safe safe, bytes memory message) public view returns (bytes32) {
return keccak256(encodeMessageDataForSafe(safe, message));
}

/**
* @notice Implementation of updated EIP-1271 signature validation method.
* @param _dataHash Hash of the data signed on the behalf of address(msg.sender)
* @param _signature Signature byte array associated with _dataHash
* @return Updated EIP1271 magic value if signature is valid, otherwise 0x0
*/
function isValidSignature(bytes32 _dataHash, bytes calldata _signature) external view returns (bytes4) {
ISignatureValidator validator = ISignatureValidator(msg.sender);
bytes4 value = validator.isValidSignature(abi.encode(_dataHash), _signature);
return (value == EIP1271_MAGIC_VALUE) ? UPDATED_MAGIC_VALUE : bytes4(0);
}

/**
* @dev Returns array of first 10 modules.
* @return Array of modules.
*/
function getModules() external view returns (address[] memory) {
// Caller should be a Safe
Safe safe = Safe(payable(msg.sender));
(address[] memory array,) = safe.getModulesPaginated(SENTINEL_MODULES, 10);
return array;
}

/**
* @dev Performs a delegatecall on a targetContract in the context of self.
* Internally reverts execution to avoid side effects (making it static). Catches revert and returns encoded result as bytes.
* @dev Inspired by https://github.com/gnosis/util-contracts/blob/bb5fe5fb5df6d8400998094fb1b32a178a47c3a1/contracts/StorageAccessible.sol
* @param targetContract Address of the contract containing the code to execute.
* @param calldataPayload Calldata that should be sent to the target contract (encoded method name and arguments).
*/
function simulate(address targetContract, bytes calldata calldataPayload)
external
returns (bytes memory response)
{
/**
* Suppress compiler warnings about not using parameters, while allowing
* parameters to keep names for documentation purposes. This does not
* generate code.
*/
targetContract;
calldataPayload;

// solhint-disable-next-line no-inline-assembly
assembly {
let internalCalldata := mload(0x40)
/**
* Store `simulateAndRevert.selector`.
* String representation is used to force right padding
*/
mstore(internalCalldata, "\xb4\xfa\xba\x09")
/**
* Abuse the fact that both this and the internal methods have the
* same signature, and differ only in symbol name (and therefore,
* selector) and copy calldata directly. This saves us approximately
* 250 bytes of code and 300 gas at runtime over the
* `abi.encodeWithSelector` builtin.
*/
calldatacopy(add(internalCalldata, 0x04), 0x04, sub(calldatasize(), 0x04))

/**
* `pop` is required here by the compiler, as top level expressions
* can't have return values in inline assembly. `call` typically
* returns a 0 or 1 value indicated whether or not it reverted, but
* since we know it will always revert, we can safely ignore it.
*/
pop(
call(
gas(),
// address() has been changed to caller() to use the implementation of the Safe
caller(),
0,
internalCalldata,
calldatasize(),
/**
* The `simulateAndRevert` call always reverts, and
* instead encodes whether or not it was successful in the return
* data. The first 32-byte word of the return data contains the
* `success` value, so write it to memory address 0x00 (which is
* reserved Solidity scratch space and OK to use).
*/
0x00,
0x20
)
)

/**
* Allocate and copy the response bytes, making sure to increment
* the free memory pointer accordingly (in case this method is
* called as an internal function). The remaining `returndata[0x20:]`
* contains the ABI encoded response bytes, so we can just write it
* as is to memory.
*/
let responseSize := sub(returndatasize(), 0x20)
response := mload(0x40)
mstore(0x40, add(response, responseSize))
returndatacopy(response, 0x20, responseSize)

if iszero(mload(0x00)) { revert(add(response, 0x20), mload(response)) }
}
}
}
3 changes: 3 additions & 0 deletions test/RumpelWallet.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,9 @@ contract RumpelWalletTest is Test {
bytes4 EIP1271_MAGIC_VALUE = 0x1626ba7e;
assertEq(safe.isValidSignature(keccak256(message), emptySignature), EIP1271_MAGIC_VALUE);

// Still verifies the signature, even if a non-empty signature is provided
assertEq(safe.isValidSignature(keccak256(message), "0x1234"), EIP1271_MAGIC_VALUE);

// Demonstrate that an incorrect message hash fails
vm.expectRevert("Hash not approved");
bytes memory incorrectMessage = "Hello Safe bad";
Expand Down

0 comments on commit 0611c27

Please sign in to comment.