From 66fce5f335db960ae460ad297830239880b24cc5 Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Mon, 2 Sep 2024 13:16:12 -0700 Subject: [PATCH 1/9] fix: make compatibility fb handler updatable --- src/RumpelWalletFactory.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/src/RumpelWalletFactory.sol b/src/RumpelWalletFactory.sol index 468c48a..d127de5 100644 --- a/src/RumpelWalletFactory.sol +++ b/src/RumpelWalletFactory.sol @@ -88,6 +88,7 @@ contract RumpelWalletFactory is Ownable, Pausable { else if (what == "RUMPEL_MODULE") rumpelModule = data; else if (what == "RUMPEL_GUARD") rumpelGuard = data; else if (what == "INITIALIZATION_SCRIPT") initializationScript = data; + else if (what == "COMPATIBILITY_FALLBACK") compatibilityFallback = data; else revert UnrecognizedParam(what); emit ParamChanged(what, data); } From 4504834cabc00a9b99508d65f3d77128886d9690 Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Tue, 3 Sep 2024 11:25:23 -0700 Subject: [PATCH 2/9] fix: improve wallet creation salt uniqueness to prevent front-running --- src/RumpelWalletFactory.sol | 14 +++++++++++--- test/RumpelWallet.t.sol | 4 +++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/RumpelWalletFactory.sol b/src/RumpelWalletFactory.sol index 468c48a..888269b 100644 --- a/src/RumpelWalletFactory.sol +++ b/src/RumpelWalletFactory.sol @@ -45,6 +45,8 @@ contract RumpelWalletFactory is Ownable, Pausable { uint256 threshold, InitializationScript.InitCall[] calldata initCalls ) external whenNotPaused returns (address) { + bytes32 salt = keccak256(abi.encodePacked(msg.sender, saltNonce[msg.sender]++)); + address safe = proxyFactory.createProxyWithNonce( safeSingleton, abi.encodeWithSelector( @@ -58,7 +60,7 @@ contract RumpelWalletFactory is Ownable, Pausable { 0, // payment address(0) // paymentReceiver ), - saltNonce[msg.sender]++ // For deterministic address generation + uint256(salt) // Use the new salt ); emit SafeCreated(safe, owners, threshold); @@ -66,8 +68,14 @@ contract RumpelWalletFactory is Ownable, Pausable { return safe; } - function precomputeAddress(bytes memory _initializer, uint256 _saltNonce) external view returns (address) { - bytes32 salt = keccak256(abi.encodePacked(keccak256(_initializer), _saltNonce)); + function precomputeAddress(bytes memory _initializer, address _sender, uint256 _saltNonce) + external + view + returns (address) + { + bytes32 salt = keccak256( + abi.encodePacked(keccak256(_initializer), uint256(keccak256(abi.encodePacked(_sender, _saltNonce)))) + ); bytes memory deploymentData = abi.encodePacked(proxyFactory.proxyCreationCode(), uint256(uint160(safeSingleton))); diff --git a/test/RumpelWallet.t.sol b/test/RumpelWallet.t.sol index 9a8b684..4587707 100644 --- a/test/RumpelWallet.t.sol +++ b/test/RumpelWallet.t.sol @@ -157,11 +157,13 @@ contract RumpelWalletTest is Test { address(0) ); + vm.startPrank(alice); uint256 saltNonce = 0; // First wallet for this sender - address expectedAddress = rumpelWalletFactory.precomputeAddress(initializer, saltNonce); + address expectedAddress = rumpelWalletFactory.precomputeAddress(initializer, alice, saltNonce); // Create the wallet address actualAddress = rumpelWalletFactory.createWallet(owners, 1, initCalls); + vm.stopPrank(); // Check if the actual address matches the expected address assertEq(actualAddress, expectedAddress, "Actual address does not match expected address"); From 5c83725d805435c51d790cc5da1bd5b1aab47bfd Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Tue, 3 Sep 2024 11:27:47 -0700 Subject: [PATCH 3/9] chore: improve nonce gen comments --- src/RumpelWalletFactory.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/RumpelWalletFactory.sol b/src/RumpelWalletFactory.sol index 888269b..e65646c 100644 --- a/src/RumpelWalletFactory.sol +++ b/src/RumpelWalletFactory.sol @@ -45,6 +45,7 @@ contract RumpelWalletFactory is Ownable, Pausable { uint256 threshold, InitializationScript.InitCall[] calldata initCalls ) external whenNotPaused returns (address) { + // Calculate a unique salt based on the sender's address and nonce. bytes32 salt = keccak256(abi.encodePacked(msg.sender, saltNonce[msg.sender]++)); address safe = proxyFactory.createProxyWithNonce( @@ -60,7 +61,7 @@ contract RumpelWalletFactory is Ownable, Pausable { 0, // payment address(0) // paymentReceiver ), - uint256(salt) // Use the new salt + uint256(salt) // For deterministic address generation ); emit SafeCreated(safe, owners, threshold); From 103884b39fe32067802dc7b76afec3e95475d866 Mon Sep 17 00:00:00 2001 From: Steven Valeri Date: Tue, 3 Sep 2024 16:07:05 -0400 Subject: [PATCH 4/9] feat: add test --- test/RumpelWallet.t.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/RumpelWallet.t.sol b/test/RumpelWallet.t.sol index 9a8b684..af0be03 100644 --- a/test/RumpelWallet.t.sol +++ b/test/RumpelWallet.t.sol @@ -90,6 +90,7 @@ contract RumpelWalletTest is Test { address newScript = makeAddr("newScript"); address newSingleton = makeAddr("newSingleton"); address newProxyFactory = makeAddr("newProxyFactory"); + address newCompatibilityFallback = makeAddr("newCompatibilityFallback"); vm.startPrank(admin); rumpelWalletFactory.setParam("RUMPEL_GUARD", newGuard); @@ -97,6 +98,7 @@ contract RumpelWalletTest is Test { rumpelWalletFactory.setParam("INITIALIZATION_SCRIPT", newScript); rumpelWalletFactory.setParam("SAFE_SINGLETON", newSingleton); rumpelWalletFactory.setParam("PROXY_FACTORY", newProxyFactory); + rumpelWalletFactory.setParam("COMPATIBILITY_FALLBACK", newCompatibilityFallback); vm.stopPrank(); assertEq(rumpelWalletFactory.rumpelGuard(), newGuard); @@ -104,6 +106,7 @@ contract RumpelWalletTest is Test { assertEq(rumpelWalletFactory.initializationScript(), newScript); assertEq(rumpelWalletFactory.safeSingleton(), newSingleton); assertEq(address(rumpelWalletFactory.proxyFactory()), newProxyFactory); + assertEq(rumpelWalletFactory.compatibilityFallback(), newCompatibilityFallback); } function testFuzz_createWalletOwners(uint256 ownersLength, uint256 threshold) public { From 28202ca6e0282d54ac150bd1ef68d254abbaf082 Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Tue, 3 Sep 2024 13:18:23 -0700 Subject: [PATCH 5/9] chore: cast salt during assignment --- src/RumpelWalletFactory.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/RumpelWalletFactory.sol b/src/RumpelWalletFactory.sol index e65646c..a217dd7 100644 --- a/src/RumpelWalletFactory.sol +++ b/src/RumpelWalletFactory.sol @@ -46,7 +46,7 @@ contract RumpelWalletFactory is Ownable, Pausable { InitializationScript.InitCall[] calldata initCalls ) external whenNotPaused returns (address) { // Calculate a unique salt based on the sender's address and nonce. - bytes32 salt = keccak256(abi.encodePacked(msg.sender, saltNonce[msg.sender]++)); + uint256 salt = uint256(keccak256(abi.encodePacked(msg.sender, saltNonce[msg.sender]++))); address safe = proxyFactory.createProxyWithNonce( safeSingleton, @@ -61,7 +61,7 @@ contract RumpelWalletFactory is Ownable, Pausable { 0, // payment address(0) // paymentReceiver ), - uint256(salt) // For deterministic address generation + salt // For deterministic address generation ); emit SafeCreated(safe, owners, threshold); From b4c46e487684d18f4c9af1eb465e9c30622ee26c Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Wed, 4 Sep 2024 12:44:24 -0700 Subject: [PATCH 6/9] chore: APGL --- script/RumpelWalletFactory.s.sol | 2 +- src/InitializationScript.sol | 2 +- src/RumpelGuard.sol | 2 +- src/RumpelModule.sol | 2 +- src/RumpelWalletFactory.sol | 2 +- src/interfaces/external/IGuard.sol | 2 +- src/interfaces/external/ISafe.sol | 2 +- src/interfaces/external/ISafeProxyFactory.sol | 2 +- src/interfaces/external/ISignMessageLib.sol | 2 +- test/RumpelWallet.t.sol | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/script/RumpelWalletFactory.s.sol b/script/RumpelWalletFactory.s.sol index 7c28c91..2c1cd3e 100644 --- a/script/RumpelWalletFactory.s.sol +++ b/script/RumpelWalletFactory.s.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; import {Script, console} from "forge-std/Script.sol"; diff --git a/src/InitializationScript.sol b/src/InitializationScript.sol index 7fdfff8..8e1317a 100644 --- a/src/InitializationScript.sol +++ b/src/InitializationScript.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; import {ISafe} from "./interfaces/external/ISafe.sol"; diff --git a/src/RumpelGuard.sol b/src/RumpelGuard.sol index dd75221..9c9e3c3 100644 --- a/src/RumpelGuard.sol +++ b/src/RumpelGuard.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; diff --git a/src/RumpelModule.sol b/src/RumpelModule.sol index cdfbe41..1c42201 100644 --- a/src/RumpelModule.sol +++ b/src/RumpelModule.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; diff --git a/src/RumpelWalletFactory.sol b/src/RumpelWalletFactory.sol index 468c48a..6e363de 100644 --- a/src/RumpelWalletFactory.sol +++ b/src/RumpelWalletFactory.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; diff --git a/src/interfaces/external/IGuard.sol b/src/interfaces/external/IGuard.sol index 81910d3..a94aec6 100644 --- a/src/interfaces/external/IGuard.sol +++ b/src/interfaces/external/IGuard.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; import {Enum} from "./ISafe.sol"; diff --git a/src/interfaces/external/ISafe.sol b/src/interfaces/external/ISafe.sol index 2cec7cf..a34936b 100644 --- a/src/interfaces/external/ISafe.sol +++ b/src/interfaces/external/ISafe.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; library Enum { diff --git a/src/interfaces/external/ISafeProxyFactory.sol b/src/interfaces/external/ISafeProxyFactory.sol index 87cb178..886782e 100644 --- a/src/interfaces/external/ISafeProxyFactory.sol +++ b/src/interfaces/external/ISafeProxyFactory.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; interface ISafeProxyFactory { diff --git a/src/interfaces/external/ISignMessageLib.sol b/src/interfaces/external/ISignMessageLib.sol index 14d9923..8694433 100644 --- a/src/interfaces/external/ISignMessageLib.sol +++ b/src/interfaces/external/ISignMessageLib.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; interface ISignMessageLib { diff --git a/test/RumpelWallet.t.sol b/test/RumpelWallet.t.sol index 9a8b684..162a5ff 100644 --- a/test/RumpelWallet.t.sol +++ b/test/RumpelWallet.t.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: AGPL-3.0-only pragma solidity ^0.8.19; import {Test, console} from "forge-std/Test.sol"; From ed19c8a73c589fe293dc8ee87a8aba4e9465dd1e Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Wed, 4 Sep 2024 12:58:01 -0700 Subject: [PATCH 7/9] chore: lock sol version to 0.8.24 --- script/RumpelWalletFactory.s.sol | 2 +- src/InitializationScript.sol | 2 +- src/RumpelGuard.sol | 2 +- src/RumpelModule.sol | 2 +- src/RumpelWalletFactory.sol | 2 +- src/interfaces/external/IGuard.sol | 2 +- src/interfaces/external/ISafe.sol | 2 +- src/interfaces/external/ISafeProxyFactory.sol | 2 +- src/interfaces/external/ISignMessageLib.sol | 2 +- test/RumpelWallet.t.sol | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/script/RumpelWalletFactory.s.sol b/script/RumpelWalletFactory.s.sol index 7c28c91..95a4af2 100644 --- a/script/RumpelWalletFactory.s.sol +++ b/script/RumpelWalletFactory.s.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; import {Script, console} from "forge-std/Script.sol"; import {PointTokenVault} from "point-tokenization-vault/PointTokenVault.sol"; diff --git a/src/InitializationScript.sol b/src/InitializationScript.sol index 7fdfff8..3788c22 100644 --- a/src/InitializationScript.sol +++ b/src/InitializationScript.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; import {ISafe} from "./interfaces/external/ISafe.sol"; import {Enum} from "./interfaces/external/ISafe.sol"; diff --git a/src/RumpelGuard.sol b/src/RumpelGuard.sol index dd75221..3527ba6 100644 --- a/src/RumpelGuard.sol +++ b/src/RumpelGuard.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; diff --git a/src/RumpelModule.sol b/src/RumpelModule.sol index cdfbe41..179bb3f 100644 --- a/src/RumpelModule.sol +++ b/src/RumpelModule.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; diff --git a/src/RumpelWalletFactory.sol b/src/RumpelWalletFactory.sol index 468c48a..7b176c6 100644 --- a/src/RumpelWalletFactory.sol +++ b/src/RumpelWalletFactory.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {Pausable} from "@openzeppelin/contracts/utils/Pausable.sol"; diff --git a/src/interfaces/external/IGuard.sol b/src/interfaces/external/IGuard.sol index 81910d3..271bd21 100644 --- a/src/interfaces/external/IGuard.sol +++ b/src/interfaces/external/IGuard.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; import {Enum} from "./ISafe.sol"; diff --git a/src/interfaces/external/ISafe.sol b/src/interfaces/external/ISafe.sol index 2cec7cf..f26f474 100644 --- a/src/interfaces/external/ISafe.sol +++ b/src/interfaces/external/ISafe.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; library Enum { enum Operation { diff --git a/src/interfaces/external/ISafeProxyFactory.sol b/src/interfaces/external/ISafeProxyFactory.sol index 87cb178..f43b87d 100644 --- a/src/interfaces/external/ISafeProxyFactory.sol +++ b/src/interfaces/external/ISafeProxyFactory.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; interface ISafeProxyFactory { function createProxyWithNonce(address _singleton, bytes memory _initializer, uint256 _saltNonce) diff --git a/src/interfaces/external/ISignMessageLib.sol b/src/interfaces/external/ISignMessageLib.sol index 14d9923..9bf1747 100644 --- a/src/interfaces/external/ISignMessageLib.sol +++ b/src/interfaces/external/ISignMessageLib.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; interface ISignMessageLib { function signMessage(bytes calldata digest) external; diff --git a/test/RumpelWallet.t.sol b/test/RumpelWallet.t.sol index 9a8b684..380e500 100644 --- a/test/RumpelWallet.t.sol +++ b/test/RumpelWallet.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; +pragma solidity =0.8.24; import {Test, console} from "forge-std/Test.sol"; import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol"; From 8a2b712571bea76674ca4192015e915b69040abb Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Thu, 5 Sep 2024 17:08:16 -0700 Subject: [PATCH 8/9] refactor: rmv ability to set value for moudle execution --- src/RumpelModule.sol | 5 ++--- test/RumpelWallet.t.sol | 5 ----- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/RumpelModule.sol b/src/RumpelModule.sol index cdfbe41..518dfa0 100644 --- a/src/RumpelModule.sol +++ b/src/RumpelModule.sol @@ -14,7 +14,6 @@ contract RumpelModule is Ownable { struct Call { ISafe safe; address to; - uint256 value; bytes data; Enum.Operation operation; } @@ -49,13 +48,13 @@ contract RumpelModule is Ownable { } } - bool success = call.safe.execTransactionFromModule(call.to, call.value, call.data, call.operation); + bool success = call.safe.execTransactionFromModule(call.to, 0, call.data, call.operation); if (!success) { revert ExecFailed(call.safe, call.to, call.data); } - emit ExecutionFromModule(call.safe, call.to, call.value, call.data); + emit ExecutionFromModule(call.safe, call.to, 0, call.data); unchecked { ++i; diff --git a/test/RumpelWallet.t.sol b/test/RumpelWallet.t.sol index 9a8b684..c4a712d 100644 --- a/test/RumpelWallet.t.sol +++ b/test/RumpelWallet.t.sol @@ -522,7 +522,6 @@ contract RumpelWalletTest is Test { safe: safe, to: address(mockToken), data: abi.encodeCall(ERC20.transfer, (RUMPEL_VAULT, 1.1e18)), - value: 0, operation: Enum.Operation.Call }); @@ -548,7 +547,6 @@ contract RumpelWalletTest is Test { safe: safe, to: address(mockToken), data: abi.encodeCall(ERC20.transfer, (RUMPEL_VAULT, 1.1e18)), - value: 0, operation: Enum.Operation.Call }); rumpelModule.exec(calls); @@ -574,7 +572,6 @@ contract RumpelWalletTest is Test { safe: safe, to: address(rumpelModule.signMessageLib()), data: abi.encodeCall(ISignMessageLib.signMessage, (abi.encode(keccak256(message)))), - value: 0, operation: Enum.Operation.DelegateCall }); rumpelModule.exec(calls); @@ -618,7 +615,6 @@ contract RumpelWalletTest is Test { safe: safe, to: address(mockToken), data: abi.encodeCall(ERC20.transfer, (RUMPEL_VAULT, 1e18)), - value: 0, operation: Enum.Operation.Call }); rumpelModule.exec(calls); @@ -646,7 +642,6 @@ contract RumpelWalletTest is Test { safe: safe, to: address(safe), data: abi.encodeCall(ISafe.enableModule, (address(newRumpelModule))), - value: 0, operation: Enum.Operation.Call }); rumpelModule.exec(calls); From 0314297a6a78de5d941cea2cc9f316dff528ecd4 Mon Sep 17 00:00:00 2001 From: Josh Levine Date: Thu, 5 Sep 2024 17:09:28 -0700 Subject: [PATCH 9/9] chore: set compatibility fb handler sol version to 0.8.24 --- src/external/CompatibilityFallbackHandler.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/external/CompatibilityFallbackHandler.sol b/src/external/CompatibilityFallbackHandler.sol index 9c96b07..3ea9f54 100644 --- a/src/external/CompatibilityFallbackHandler.sol +++ b/src/external/CompatibilityFallbackHandler.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: LGPL-3.0-only -pragma solidity >=0.7.0 <0.9.0; +pragma solidity =0.8.24; import "safe-smart-account/contracts/handler/TokenCallbackHandler.sol"; import "safe-smart-account/contracts/Safe.sol";