Merge pull request #67 from keyper-labs/feature/review-pre-audit

Feature/review pre audit
keyper-labs · May 27, 2024 · 70923a6 · 70923a6
2 parents db81958 + 4e54e9e
commit 70923a6
Show file tree

Hide file tree

Showing 22 changed files with 542 additions and 1,567 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
diff --git a/Makefile b/Makefile
@@ -48,10 +48,10 @@ deploy-keyper-env-polygon :; source .env && forge script script/DeployKeyperEnv.
 deploy-keyper-env :; source .env && forge script script/DeployKeyperEnv.s.sol:DeployKeyperEnv --rpc-url ${SEPOLIA_RPC_URL}  --private-key ${PRIVATE_KEY} --broadcast --verify --etherscan-api-key ${ETHERSCAN_KEY} -vvvv
 
 # Deploy module in fork-polygon
-deploy-keyper-env-fork-polygon :; source .env && forge script script/DeployKeyperEnv.s.sol:DeployKeyperEnv --fork-url http://localhost:8545  --private-key ${PRIVATE_KEY}
+deploy-keyper-env-fork-polygon :; source .env && forge script script/DeployKeyperEnv.s.sol:DeployKeyperEnv --fork-url ${POLYGON_RPC_URL}  --private-key ${PRIVATE_KEY}
 
 # Deploy New Safe
 deploy-new-safe :; source .env && forge script script/DeployKeyperSafe.t.sol:DeployKeyperSafe --rpc-url ${GOERLI_RPC_URL}  --private-key ${PRIVATE_KEY} --broadcast -vvvv
 
 # Run Unit-Test in Fork polygon
-test-fork-polygon :; source .env && forge script script/SkipExecutionOnBehalf.s.sol:SkipSeveralScenarios --fork-url http://localhost:8545  --private-key ${PRIVATE_KEY} --broadcast -vvvv
+test-fork-polygon :; source .env && forge script script/SkipExecutionOnBehalf.s.sol:SkipSeveralScenarios --fork-url ${POLYGON_RPC_URL}  --private-key ${PRIVATE_KEY} --broadcast -vvvv
diff --git a/script/DeployKeyperEnv.s.sol b/script/DeployKeyperEnv.s.sol
@@ -11,7 +11,8 @@ import {GnosisSafeProxyFactory} from
     "@safe-contracts/proxies/GnosisSafeProxyFactory.sol";
 import {GnosisSafe} from "@safe-contracts/GnosisSafe.sol";
 
-// Deployement of Gnosis Safe contracts, KeyperRoles and KeyperModule
+/// Deployement of Gnosis Safe contracts, KeyperRoles and KeyperModule
+/// @custom:security-contact [email protected]
 contract DeployKeyperEnv is Script {
     function run() public {
         vm.startBroadcast();

diff --git a/script/DeployKeyperRoles.t.sol b/script/DeployKeyperRoles.t.sol
@@ -5,8 +5,8 @@ import "forge-std/Script.sol";
 import {Auth, Authority} from "@solmate/auth/Auth.sol";
 import "../src/KeyperRoles.sol";
 
-// import "@solenv/Solenv.sol";
-
+/// @title Deploy KeyperRoles
+/// @custom:security-contact [email protected]
 contract DeployKeyperRoles is Script {
     function run() public {
         // Solenv.config();

diff --git a/script/DeployKeyperSafe.t.sol b/script/DeployKeyperSafe.t.sol
@@ -2,10 +2,11 @@
 pragma solidity ^0.8.15;
 
 import "forge-std/Script.sol";
-import "forge-std/console.sol";
 import {KeyperModule} from "../src/KeyperModule.sol";
 import "@solenv/Solenv.sol";
 
+/// @title Deploy KeyperSafe
+/// @custom:security-contact [email protected]
 contract DeployKeyperSafe is Script {
     function run() public {
         Solenv.config();

diff --git a/script/DeployLibraries.s.sol b/script/DeployLibraries.s.sol
@@ -8,6 +8,8 @@ import {DataTypes} from "../libraries/DataTypes.sol";
 import {Errors} from "../libraries/Errors.sol";
 import {Events} from "../libraries/Events.sol";
 
+/// @title Deploy Libraries
+/// @custom:security-contact [email protected]
 contract DeployLibraries is Script {
     function run() public {
         vm.startBroadcast();

diff --git a/script/DeployModuleWithMockedSafe.t.sol b/script/DeployModuleWithMockedSafe.t.sol
@@ -7,6 +7,8 @@ import "test/mocks/MockedContract.t.sol";
 import {CREATE3Factory} from "@create3/CREATE3Factory.sol";
 import "@solenv/Solenv.sol";
 
+/// Deployement of Gnosis Safe contracts, KeyperRoles and KeyperModule
+/// @custom:security-contact [email protected]
 contract DeployModuleWithMockedSafe is Script {
     function run() public {
         Solenv.config();

diff --git a/script/SkipExecutionOnBehalf.s.sol b/script/SkipExecutionOnBehalf.s.sol
@@ -6,24 +6,26 @@ import "../src/SigningUtils.sol";
 import "../test/helpers/SkipSetupEnv.s.sol";
 import {Errors} from "../libraries/Errors.sol";
 
+/// @title Several Scenarios of Execution On Behalf in live Mainnet/Testnet (Polygon and Sepolia)
+/// @custom:security-contact [email protected]
 contract SkipSeveralScenarios is Script, SkipSetupEnv {
     /// Setup the Environment, with run() from SkipSetupEnvGoerli
     function setUp() public {
         // Set up env
         run();
-        TestExecutionOnBehalf(); // ✅
-            // These are different chain test scenarios, specifically for the execTransactionOnBehalf function in Polygon and Sepolia.
-            // We test each scenario independently manually and get the results on the Live Mainnet on Polygon and Sepolia.
-            // testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_SAFE(); // ✅
-            // testCan_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree(); // ✅
-            // testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES(); // ✅
-            // testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree(); // ✅
-            // testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_WRONG_SIGNATURES(); // ✅
-            // testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_INVALID_SIGNATURES(); // ✅
-            // testRevertInvalidSignatureExecOnBehalf(); // ✅
-            // testRevertInvalidAddressProvidedExecTransactionOnBehalfScenarioOne(); // ✅
-            // testRevertZeroAddressProvidedExecTransactionOnBehalfScenarioTwo(); // ✅
-            // testRevertSuperSafeExecOnBehalf(); // ✅
+        testRevertInvalidGnosisSafeExecTransactionOnBehalf();
+        // These are different chain test scenarios, specifically for the execTransactionOnBehalf function in Polygon and Sepolia.
+        // We test each scenario independently manually and get the results on the Live Mainnet on Polygon and Sepolia.
+        // testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_SAFE(); // ✅
+        // testCan_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree(); // ✅
+        // testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES(); // ✅
+        // testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree(); // ✅
+        // testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_WRONG_SIGNATURES(); // ✅
+        // testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_INVALID_SIGNATURES(); // ✅
+        // testRevertInvalidSignatureExecOnBehalf(); // ✅
+        // testRevertInvalidAddressProvidedExecTransactionOnBehalfScenarioOne(); // ✅
+        // testRevertZeroAddressProvidedExecTransactionOnBehalfScenarioTwo(); // ✅
+        // testRevertSuperSafeExecOnBehalf(); // ✅
     }
 
     // Test Execution On Behalf
@@ -175,9 +177,6 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
 
         address payable rootAddr =
             payable(keyperModule.getSquadSafeAddress(rootId));
-        address payable safeSquadA1Addr =
-            payable(keyperModule.getSquadSafeAddress(safeSquadA1));
-        address payable receiverAddr = payable(receiver);
         address childSquadA1Addr = newKeyperSafe(4, 2);
         bool result = createAddSquadTx(safeSquadA1, "ChildSquadA1");
         assertEq(result, true);
@@ -210,6 +209,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
 
         // Set keyperhelper gnosis safe to rootAddr
         setGnosisSafe(childSquadA1Addr);
+        // Try to execute on behalf function from a not authorized caller child Squad A1 over Root Safe
         bytes memory signatures2 = encodeSignaturesKeyperTx(
             orgHash,
             childSquadA1Addr,
@@ -219,6 +219,8 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             emptyData,
             Enum.Operation(0)
         );
+        // try to encode the end of the signature, from EOA random the internal data to the Wrapper of
+        // Execution On Behalf, with a rogue caller and that is secondary squad A1 over Root Safe
         bytes memory internalData = abi.encodeWithSignature(
             "execTransactionOnBehalf(bytes32,address,address,address,uint256,bytes,uint8,bytes)",
             orgHash,
@@ -261,11 +263,9 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         console.log("Root address Test Execution On Behalf: ", rootAddr);
         console.log("Safe Squad A1 Test Execution On Behalf: ", safeSquadA1Addr);
 
-        // tx ETH from msg.sender to rootAddr
+        // tx ETH from msg.sender to safeSquadA1Addr
         (bool sent, bytes memory data) = safeSquadA1Addr.call{value: 2 gwei}("");
         require(sent, "Failed to send Ether");
-        // (sent, data) = receiverAddr.call{value: 1e5 gwei}("");
-        // require(sent, "Failed to send Ether");
 
         // Set keyperhelper gnosis safe to org
         setGnosisSafe(rootAddr);
@@ -280,7 +280,6 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             Enum.Operation(0)
         );
         // Execute on behalf function
-
         bool result = execTransactionOnBehalfTx(
             orgHash,
             rootAddr,
@@ -324,7 +323,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         require(sent, "Failed to send Ether");
 
         // Random wallet instead of a safe (EOA)
-        address callerEOA = address(receiver);
+        address callerEOA = address(msg.sender);
 
         // Owner of Root Safe sign args
         setGnosisSafe(rootAddr);
@@ -453,7 +452,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         require(sent, "Failed to send Ether");
 
         // Random wallet instead of a safe (EOA)
-        address callerEOA = address(0xFED);
+        address callerEOA = address(msg.sender);
 
         // Owner of Target Safe signed args
         setGnosisSafe(safeSubSquadA1Addr);
@@ -468,8 +467,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             Enum.Operation(0)
         );
 
-        // vm.startPrank(callerEOA);
-        vm.expectRevert("GS020");
+        // GS020: Invalid signatures provided
         keyperModule.execTransactionOnBehalf(
             orgHash,
             rootAddr,
@@ -510,7 +508,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         (sent, data) = safeSubSquadA1Addr.call{value: 100 gwei}("");
         require(sent, "Failed to send Ether");
         // Random wallet instead of a safe (EOA)
-        address callerEOA = address(0xFED);
+        address callerEOA = address(msg.sender);
 
         // Owner of Root Safe sign args
         setGnosisSafe(rootAddr);
@@ -525,8 +523,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             Enum.Operation(0)
         );
 
-        // vm.startPrank(callerEOA);
-        vm.expectRevert("GS026");
+        // GS026: Invalid signatures provided
         keyperModule.execTransactionOnBehalf(
             orgHash,
             rootAddr,
@@ -584,7 +581,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         );
 
         setGnosisSafe(safeSubSquadA1Addr);
-        // vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector);
+        // NotAuthorizedExecOnBehalf: Caller is not authorized to execute on behalf
         bool result = execTransactionOnBehalfTx(
             orgHash,
             safeSubSquadA1Addr,
@@ -628,7 +625,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             Enum.Operation(0)
         );
 
-        // vm.expectRevert("GS013");
+        // GS013: Invalid signatures provided
         execTransactionOnBehalfTx(
             orgHash,
             rootAddr,
@@ -642,16 +639,16 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         vm.stopBroadcast();
     }
 
-    // // Revert ZeroAddressProvided() execTransactionOnBehalf when arg "to" is address(0)
-    // // Scenario 1
-    // // Caller: rootAddr (org)
-    // // Caller Type: rootSafe
-    // // Caller Role: ROOT_SAFE
-    // // TargerSafe: safeSquadA1
-    // // TargetSafe Type: safe as a Child
-    // //            rootSafe -----------
-    // //               |                |
-    // //           safeSquadA1 <--------
+    // Revert ZeroAddressProvided() execTransactionOnBehalf when arg "to" is address(0)
+    // Scenario 1
+    // Caller: rootAddr (org)
+    // Caller Type: rootSafe
+    // Caller Role: ROOT_SAFE
+    // TargerSafe: safeSquadA1
+    // TargetSafe Type: safe as a Child
+    //            rootSafe -----------
+    //               |                |
+    //           safeSquadA1 <--------
     function testRevertInvalidAddressProvidedExecTransactionOnBehalfScenarioOne(
     ) public {
         vm.startBroadcast();
@@ -676,8 +673,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             emptyData,
             Enum.Operation(0)
         );
-
-        vm.expectRevert(Errors.InvalidAddressProvided.selector);
+        // Execute on behalf function with invalid receiver
         keyperModule.execTransactionOnBehalf(
             orgHash,
             rootAddr,
@@ -691,16 +687,16 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         vm.stopBroadcast();
     }
 
-    // // Revert ZeroAddressProvided() execTransactionOnBehalf when param "targetSafe" is address(0)
-    // // Scenario 2
-    // // Caller: rootAddr (org)
-    // // Caller Type: rootSafe
-    // // Caller Role: ROOT_SAFE
-    // // TargerSafe: safeSquadA1
-    // // TargetSafe Type: safe as a Child
-    // //            rootSafe -----------
-    // //               |                |
-    // //           safeSquadA1 <--------
+    // Revert ZeroAddressProvided() execTransactionOnBehalf when param "targetSafe" is address(0)
+    // Scenario 2
+    // Caller: rootAddr (org)
+    // Caller Type: rootSafe
+    // Caller Role: ROOT_SAFE
+    // TargerSafe: safeSquadA1
+    // TargetSafe Type: safe as a Child
+    //            rootSafe -----------
+    //               |                |
+    //           safeSquadA1 <--------
     function testRevertZeroAddressProvidedExecTransactionOnBehalfScenarioTwo()
         public
     {
@@ -726,13 +722,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             Enum.Operation(0)
         );
 
-        // Execute on behalf function from a not authorized caller
-        // vm.startPrank(rootAddr);
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                Errors.InvalidGnosisSafe.selector, address(0)
-            )
-        );
+        // InvalidGnosisSafe: Invalid Gnosis Safe
         keyperModule.execTransactionOnBehalf(
             orgHash,
             rootAddr,
@@ -746,16 +736,16 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         vm.stopBroadcast();
     }
 
-    // // Revert ZeroAddressProvided() execTransactionOnBehalf when param "org" is address(0)
-    // // Scenario 3
-    // // Caller: rootAddr (org)
-    // // Caller Type: rootSafe
-    // // Caller Role: ROOT_SAFE
-    // // TargerSafe: safeSquadA1
-    // // TargetSafe Type: safe as a Child
-    // //            rootSafe -----------
-    // //               |                |
-    // //           safeSquadA1 <--------
+    // Revert ZeroAddressProvided() execTransactionOnBehalf when param "org" is address(0)
+    // Scenario 3
+    // Caller: rootAddr (org)
+    // Caller Type: rootSafe
+    // Caller Role: ROOT_SAFE
+    // TargerSafe: safeSquadA1
+    // TargetSafe Type: safe as a Child
+    //            rootSafe -----------
+    //               |                |
+    //           safeSquadA1 <--------
     function testRevertOrgNotRegisteredExecTransactionOnBehalfScenarioThree()
         public
     {
@@ -780,10 +770,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             emptyData,
             Enum.Operation(0)
         );
-        // Execute on behalf function from a not authorized caller
-        vm.expectRevert(
-            abi.encodeWithSelector(Errors.OrgNotRegistered.selector, address(0))
-        );
+        // OrgNotRegistered: Org not registered
         keyperModule.execTransactionOnBehalf(
             bytes32(0),
             rootAddr,
@@ -797,12 +784,12 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
         vm.stopBroadcast();
     }
 
-    // // Revert InvalidGnosisSafe() execTransactionOnBehalf : when param "targetSafe" is not a safe
-    // // Caller: rootAddr (org)
-    // // Caller Type: rootSafe
-    // // Caller Role: ROOT_SAFE, SAFE_LEAD
-    // // TargerSafe: fakeTargetSafe
-    // // TargetSafe Type: EOA
+    // Revert InvalidGnosisSafe() execTransactionOnBehalf : when param "targetSafe" is not a safe
+    // Caller: rootAddr (org)
+    // Caller Type: rootSafe
+    // Caller Role: ROOT_SAFE, SAFE_LEAD
+    // TargerSafe: fakeTargetSafe
+    // TargetSafe Type: EOA
     function testRevertInvalidGnosisSafeExecTransactionOnBehalf() public {
         vm.startBroadcast();
         (uint256 rootId, uint256 safeSquadA1) =
@@ -826,13 +813,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv {
             emptyData,
             Enum.Operation(0)
         );
-        // Execute on behalf function from a not authorized caller
-
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                Errors.InvalidGnosisSafe.selector, fakeTargetSafe
-            )
-        );
+        // InvalidGnosisSafe: Invalid Gnosis Safe
         keyperModule.execTransactionOnBehalf(
             orgHash,
             rootAddr,

diff --git a/src/Helpers.sol b/src/Helpers.sol
@@ -16,7 +16,7 @@ import {
 } from "./DenyHelper.sol";
 import {SignatureDecoder} from "@safe-contracts/common/SignatureDecoder.sol";
 
-/// @title DenyHelper
+/// @title Helpers
 /// @custom:security-contact [email protected]
 abstract contract Helpers is DenyHelper, SignatureDecoder {
     using GnosisSafeMath for uint256;