From 32e378db36dd27bc95c9ffce197ce516400d81c5 Mon Sep 17 00:00:00 2001 From: Alfredo Lopez Date: Mon, 13 May 2024 19:31:57 +0200 Subject: [PATCH 1/4] FIX: fixed visibility of function into unit-test Reentrancy Attack Helper --- .gas-snapshot | 4 ++-- test/helpers/ReentrancyAttackHelper.t.sol | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 84804c0..958100d 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -34,7 +34,7 @@ ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TA ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES() (gas: 2415824) ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Inverse_WAY() (gas: 2597503) ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way() (gas: 2597347) -ExecTransactionOnBehalf:testReentrancyAttack() (gas: 6554041) +ExecTransactionOnBehalf:testReentrancyAttack() (gas: 6554038) ExecTransactionOnBehalf:testRevertInvalidAddressProvidedExecTransactionOnBehalfScenarioOne() (gas: 1655688) ExecTransactionOnBehalf:testRevertInvalidGnosisSafeExecTransactionOnBehalf() (gas: 1647227) ExecTransactionOnBehalf:testRevertInvalidSignatureExecOnBehalf() (gas: 1770030) @@ -133,7 +133,7 @@ ModifySafeOwners:testRevertSafeNotRegisteredRemoveOwner_EOA_Caller() (gas: 50526 ModifySafeOwners:testRevertSafeNotRegisteredRemoveOwner_SAFE_Caller() (gas: 509494) ModifySafeOwners:testRevertZeroAddressAddOwnerWithThreshold() (gas: 1578192) ModifySafeOwners:testRevertZeroAddressProvidedRemoveOwner() (gas: 2985563) -TestDeploy:testDeploy() (gas: 6172448) +TestDeploy:testDeploy() (gas: 6257319) TestDeploySafe:testTransferFundsSafe() (gas: 109672) TestEnableGuard:testEnableKeyperGuard() (gas: 120880) TestEnableGuard:testEnableKeyperModule() (gas: 122295) diff --git a/test/helpers/ReentrancyAttackHelper.t.sol b/test/helpers/ReentrancyAttackHelper.t.sol index 7b3655d..5ece6c1 100644 --- a/test/helpers/ReentrancyAttackHelper.t.sol +++ b/test/helpers/ReentrancyAttackHelper.t.sol @@ -37,7 +37,7 @@ contract AttackerHelper is Test, SignDigestHelper, SignersHelper { uint256 value, bytes memory data, Enum.Operation operation - ) public returns (bytes memory) { + ) public view returns (bytes memory) { uint256 nonce = keyper.nonce(); bytes32 txHashed = keyper.getTransactionHash( org, superSafe, targetSafe, to, value, data, operation, nonce From 6822807f669e8e5ad9d17d5c708f274f93bdae4b Mon Sep 17 00:00:00 2001 From: Alfredo Lopez Date: Tue, 14 May 2024 22:14:06 +0200 Subject: [PATCH 2/4] FIX: fix code commented, or comments not right, and additional comment for test in live mainnet / testnet (polygon and Sepolia) --- .gas-snapshot | 220 ++++++++++++------------- Makefile | 4 +- script/SkipExecutionOnBehalf.s.sol | 85 +++++----- test/ExecTransactionOnBehalf.t.sol | 235 ++++++++++++++++++++++++--- test/Hierarchies.t.sol | 4 +- test/KeyperGuard.t.sol | 16 +- test/KeyperRoles.t.sol | 2 +- test/KeyperSafe.t.sol | 12 +- test/ModifySafeOwners.t.sol | 8 +- test/helpers/KeyperSafeBuilder.t.sol | 19 ++- 10 files changed, 397 insertions(+), 208 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 958100d..a244163 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -18,140 +18,142 @@ EventsChekers:testEventWhenAddSquad() (gas: 1335323) EventsChekers:testEventWhenAddToList() (gas: 986983) EventsChekers:testEventWhenDisconnectSafe() (gas: 1326455) EventsChekers:testEventWhenDropFromList() (gas: 994365) -EventsChekers:testEventWhenExecutionOnBehalf() (gas: 1795431) +EventsChekers:testEventWhenExecutionOnBehalf() (gas: 1889961) EventsChekers:testEventWhenPromoteRootSafe() (gas: 2031889) EventsChekers:testEventWhenRegisterRootOrg() (gas: 732761) EventsChekers:testEventWhenRegisterRootSafe() (gas: 1342851) EventsChekers:testEventWhenRemoveSquad() (gas: 1390591) -EventsChekers:testEventWhenRemoveWholeTree() (gas: 2442834) +EventsChekers:testEventWhenRemoveWholeTree() (gas: 2519292) EventsChekers:testEventWhenUpdateNewLimit() (gas: 1373528) EventsChekers:testEventWhenUpdateSuper() (gas: 2451882) -ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_ROOT_SAFE_and_Target_Root_SameTree_2_levels() (gas: 2475954) -ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree() (gas: 1851393) -ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 1770471) -ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 3722505) -ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree() (gas: 2486953) -ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES() (gas: 2415824) -ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Inverse_WAY() (gas: 2597503) -ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way() (gas: 2597347) -ExecTransactionOnBehalf:testReentrancyAttack() (gas: 6554038) -ExecTransactionOnBehalf:testRevertInvalidAddressProvidedExecTransactionOnBehalfScenarioOne() (gas: 1655688) -ExecTransactionOnBehalf:testRevertInvalidGnosisSafeExecTransactionOnBehalf() (gas: 1647227) -ExecTransactionOnBehalf:testRevertInvalidSignatureExecOnBehalf() (gas: 1770030) -ExecTransactionOnBehalf:testRevertOrgNotRegisteredExecTransactionOnBehalfScenarioThree() (gas: 1662704) -ExecTransactionOnBehalf:testRevertSuperSafeExecOnBehalf() (gas: 2936605) -ExecTransactionOnBehalf:testRevertZeroAddressProvidedExecTransactionOnBehalfScenarioTwo() (gas: 1644950) -ExecTransactionOnBehalf:testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_INVALID_SIGNATURES() (gas: 2346360) -ExecTransactionOnBehalf:testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_WRONG_SIGNATURES() (gas: 2330032) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_ROOT_SAFE_and_Target_Root_SameTree_2_levels() (gas: 2572802) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree() (gas: 1945923) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 1871223) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 3818078) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree() (gas: 2583785) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES() (gas: 2512650) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Inverse_WAY() (gas: 2694314) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way() (gas: 2694158) +ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way_from_superSafe() (gas: 2696859) +ExecTransactionOnBehalf:testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_SAFE() (gas: 3305716) +ExecTransactionOnBehalf:testReentrancyAttack() (gas: 6554082) +ExecTransactionOnBehalf:testRevertInvalidAddressProvidedExecTransactionOnBehalfScenarioOne() (gas: 1750262) +ExecTransactionOnBehalf:testRevertInvalidGnosisSafeExecTransactionOnBehalf() (gas: 1741757) +ExecTransactionOnBehalf:testRevertInvalidSignatureExecOnBehalf() (gas: 1864582) +ExecTransactionOnBehalf:testRevertOrgNotRegisteredExecTransactionOnBehalfScenarioThree() (gas: 1757234) +ExecTransactionOnBehalf:testRevertSuperSafeExecOnBehalf() (gas: 3032242) +ExecTransactionOnBehalf:testRevertZeroAddressProvidedExecTransactionOnBehalfScenarioTwo() (gas: 1739524) +ExecTransactionOnBehalf:testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_INVALID_SIGNATURES() (gas: 2443176) +ExecTransactionOnBehalf:testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_WRONG_SIGNATURES() (gas: 2426869) GnosisSafeHelper:testNewKeyperSafe() (gas: 9403967) -Hierarchies:testAddSquad() (gas: 1606035) -Hierarchies:testAddSubSquad() (gas: 2223775) -Hierarchies:testCreateSquadThreeTiersTree() (gas: 2206527) +Hierarchies:testAddSquad() (gas: 1700565) +Hierarchies:testAddSubSquad() (gas: 2318305) +Hierarchies:testCreateSquadThreeTiersTree() (gas: 2303268) Hierarchies:testExpectInvalidSquadId() (gas: 14380) Hierarchies:testExpectSquadNotRegistered() (gas: 16790) -Hierarchies:testIsSuperSafe() (gas: 2834006) -Hierarchies:testOrgFourTiersTreeSuperSafeRoles() (gas: 2823357) +Hierarchies:testIsSuperSafe() (gas: 2929578) +Hierarchies:testOrgFourTiersTreeSuperSafeRoles() (gas: 2918929) Hierarchies:testRegisterRootOrg() (gas: 361277) -Hierarchies:testRevertIfTryInvalidLimit() (gas: 1654058) -Hierarchies:testRevertIfTryNotRootSafe() (gas: 4297152) -Hierarchies:testRevertSafeAlreadyRegisteredAddSquad() (gas: 2236708) -Hierarchies:testRevertUpdateSuperIfCallerIsNotSafe() (gas: 1558089) -Hierarchies:testRevertUpdateSuperIfCallerNotPartofTheOrg() (gas: 3001130) -Hierarchies:testRevertUpdateSuperInvalidSquadId() (gas: 1576957) -Hierarchies:testRevertifExceedMaxDepthTreeLimit() (gas: 5718097) -Hierarchies:testRevertifExceedMaxDepthTreeLimitAndUpdateSuper() (gas: 7882279) -Hierarchies:testRevertifUpdateDepthTreeLimitAndUpdateSuperToAnotherOrg() (gas: 12255110) -Hierarchies:testRevertifUpdateLimitAndExceedMaxDepthTreeLimit() (gas: 10665785) -Hierarchies:testRevertifUpdateLimitAndExceedMaxDepthTreeLimitAndUpdateSuper() (gas: 12895967) -Hierarchies:testRevertifUpdateSuperToAnotherOrg() (gas: 7211539) -Hierarchies:testTreeOrgsTreeMember() (gas: 3669067) -Hierarchies:testUpdateSuper() (gas: 3554012) -KeyperGuardTest:testCanPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE() (gas: 2405328) -KeyperGuardTest:testCanRemoveWholeTree() (gas: 3942416) -KeyperGuardTest:testCanRemoveWholeTreeFourthLevel() (gas: 3025464) -KeyperGuardTest:testCannotDisableKeyperGuardAfterRemoveSquad() (gas: 1794072) -KeyperGuardTest:testCannotDisableKeyperGuardIfGuardEnabled() (gas: 1697462) -KeyperGuardTest:testCannotDisableKeyperModuleAfterRemoveSquad() (gas: 1732011) -KeyperGuardTest:testCannotDisableKeyperModuleIfGuardEnabled() (gas: 1687833) -KeyperGuardTest:testCannotDisconnectSafe_As_ROOTSAFE_TARGET_ITSELF_If_Have_children() (gas: 1647423) -KeyperGuardTest:testCannotDisconnectSafe_As_ROOTSAFE_TARGET_ROOTSAFE_SAME_TREE() (gas: 2231840) -KeyperGuardTest:testCannotDisconnectSafe_As_RootSafe_As_DifferentTree() (gas: 4579441) -KeyperGuardTest:testCannotDisconnectSafe_As_SafeLead_As_EOA() (gas: 2285298) -KeyperGuardTest:testCannotDisconnectSafe_As_SafeLead_As_SAFE() (gas: 2688610) -KeyperGuardTest:testCannotDisconnectSafe_As_SuperSafe_As_DifferentTree() (gas: 4550851) -KeyperGuardTest:testCannotDisconnectSafe_As_SuperSafe_As_SameTree() (gas: 2326725) -KeyperGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SQUAD_SAFE() (gas: 2335362) -KeyperGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE_ANOTHER_ORG() (gas: 4545814) -KeyperGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE_ANOTHER_TREE() (gas: 4465460) -KeyperGuardTest:testCannotReplayAttackDisconnectedSafe() (gas: 1612121) -KeyperGuardTest:testCannotReplayAttackRemoveSquad() (gas: 1689280) -KeyperGuardTest:testCannotReplayAttackRemoveWholeTree() (gas: 3018712) +Hierarchies:testRevertIfTryInvalidLimit() (gas: 1748588) +Hierarchies:testRevertIfTryNotRootSafe() (gas: 4397254) +Hierarchies:testRevertSafeAlreadyRegisteredAddSquad() (gas: 2331238) +Hierarchies:testRevertUpdateSuperIfCallerIsNotSafe() (gas: 1652619) +Hierarchies:testRevertUpdateSuperIfCallerNotPartofTheOrg() (gas: 3100190) +Hierarchies:testRevertUpdateSuperInvalidSquadId() (gas: 1671487) +Hierarchies:testRevertifExceedMaxDepthTreeLimit() (gas: 5813669) +Hierarchies:testRevertifExceedMaxDepthTreeLimitAndUpdateSuper() (gas: 7976810) +Hierarchies:testRevertifUpdateDepthTreeLimitAndUpdateSuperToAnotherOrg() (gas: 12355212) +Hierarchies:testRevertifUpdateLimitAndExceedMaxDepthTreeLimit() (gas: 10761357) +Hierarchies:testRevertifUpdateLimitAndExceedMaxDepthTreeLimitAndUpdateSuper() (gas: 12990498) +Hierarchies:testRevertifUpdateSuperToAnotherOrg() (gas: 7311641) +Hierarchies:testTreeOrgsTreeMember() (gas: 3770336) +Hierarchies:testUpdateSuper() (gas: 3649585) +KeyperGuardTest:testCanPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE() (gas: 2502068) +KeyperGuardTest:testCanRemoveWholeTree() (gas: 4036947) +KeyperGuardTest:testCanRemoveWholeTreeFourthLevel() (gas: 3101922) +KeyperGuardTest:testCannotDisableKeyperGuardAfterRemoveSquad() (gas: 1888602) +KeyperGuardTest:testCannotDisableKeyperGuardIfGuardEnabled() (gas: 1791992) +KeyperGuardTest:testCannotDisableKeyperModuleAfterRemoveSquad() (gas: 1826541) +KeyperGuardTest:testCannotDisableKeyperModuleIfGuardEnabled() (gas: 1782363) +KeyperGuardTest:testCannotDisconnectSafe_As_ROOTSAFE_TARGET_ITSELF_If_Have_children() (gas: 1741953) +KeyperGuardTest:testCannotDisconnectSafe_As_ROOTSAFE_TARGET_ROOTSAFE_SAME_TREE() (gas: 2328580) +KeyperGuardTest:testCannotDisconnectSafe_As_RootSafe_As_DifferentTree() (gas: 4678504) +KeyperGuardTest:testCannotDisconnectSafe_As_SafeLead_As_EOA() (gas: 2382038) +KeyperGuardTest:testCannotDisconnectSafe_As_SafeLead_As_SAFE() (gas: 2785356) +KeyperGuardTest:testCannotDisconnectSafe_As_SuperSafe_As_DifferentTree() (gas: 4649914) +KeyperGuardTest:testCannotDisconnectSafe_As_SuperSafe_As_SameTree() (gas: 2423464) +KeyperGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SQUAD_SAFE() (gas: 2432107) +KeyperGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE_ANOTHER_ORG() (gas: 4644877) +KeyperGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE_ANOTHER_TREE() (gas: 4559991) +KeyperGuardTest:testCannotReplayAttackDisconnectedSafe() (gas: 1706651) +KeyperGuardTest:testCannotReplayAttackRemoveSquad() (gas: 1783810) +KeyperGuardTest:testCannotReplayAttackRemoveWholeTree() (gas: 3095171) KeyperGuardTest:testDisableKeyperGuard() (gas: 133253) KeyperGuardTest:testDisableKeyperModule() (gas: 104822) -KeyperGuardTest:testDisconnectSafeBeforeToRemoveSquad_One_Level() (gas: 2202661) -KeyperGuardTest:testDisconnectSafeBeforeToRemoveSquad_Two_Level() (gas: 2832465) -KeyperGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_ITSELF_If_Not_Have_children() (gas: 1808142) -KeyperGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_ROOT_SAFE() (gas: 2294641) -KeyperGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_SUPERSAFE_SAME_TREE() (gas: 1608727) +KeyperGuardTest:testDisconnectSafeBeforeToRemoveSquad_One_Level() (gas: 2299401) +KeyperGuardTest:testDisconnectSafeBeforeToRemoveSquad_Two_Level() (gas: 2928037) +KeyperGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_ITSELF_If_Not_Have_children() (gas: 1902672) +KeyperGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_ROOT_SAFE() (gas: 2391380) +KeyperGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_SUPERSAFE_SAME_TREE() (gas: 1703257) KeyperRoleDeployTest:testSetUserRoles() (gas: 32278) KeyperRoleDeployTest:testSetupRolesCapabilities() (gas: 157803) KeyperRolesTest:testCan_KeyperModule_Setup_RoleContract() (gas: 18046) KeyperRolesTest:testCan_ROOT_SAFE_SetRole_ROOT_SAFE_When_RegisterOrg() (gas: 261578) -KeyperRolesTest:testCan_ROOT_SAFE_SetRole_SAFE_LEAD_to_EAO() (gas: 1660461) -KeyperRolesTest:testCan_ROOT_SAFE_SetRole_SAFE_LEAD_to_SAFE() (gas: 2098236) -KeyperRolesTest:testCannot_ROOT_SAFE_SetRole_ROOT_SAFE_to_EAO() (gas: 1585501) -KeyperRolesTest:testCannot_ROOT_SAFE_SetRole_ROOT_SAFE_to_EOA_DifferentTree_Safe() (gas: 2918446) -KeyperRolesTest:testCannot_ROOT_SAFE_SetRole_SUPER_SAFE_to_EAO() (gas: 1585386) -KeyperRolesTest:testCannot_ROOT_SAFE_SetRole_SUPER_SAFE_to_SAFE() (gas: 2206350) -KeyperRolesTest:testCannot_SUPER_SAFE_SetRole_SAFE_LEAD_to_EAO() (gas: 1584838) -ModifySafeOwners:testCan_AddOwnerWithThreshold_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2338369) -ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_EOA_is_TARGETS_LEAD() (gas: 1744621) -ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_SAFE_is_TARGETS_LEAD() (gas: 3159949) -ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 3086783) -ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 3160227) -ModifySafeOwners:testCan_AddOwnerWithThreshold_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 2349281) -ModifySafeOwners:testCan_RemoveOwner_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2294390) -ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_EOA_is_TARGETS_LEAD() (gas: 1711964) -ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_SAFE_is_TARGETS_LEAD() (gas: 3120102) -ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 1716553) -ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 2448180) -ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD_DifferentTree() (gas: 3101546) -ModifySafeOwners:testCan_RemoveOwner_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 2305221) -ModifySafeOwners:testRevertInvalidThresholdAddOwnerWithThreshold() (gas: 1784227) -ModifySafeOwners:testRevertInvalidThresholdRemoveOwner() (gas: 1755532) -ModifySafeOwners:testRevertOwnerAlreadyExistsAddOwnerWithThreshold() (gas: 1700376) +KeyperRolesTest:testCan_ROOT_SAFE_SetRole_SAFE_LEAD_to_EAO() (gas: 1754991) +KeyperRolesTest:testCan_ROOT_SAFE_SetRole_SAFE_LEAD_to_SAFE() (gas: 2192766) +KeyperRolesTest:testCannot_ROOT_SAFE_SetRole_ROOT_SAFE_to_EAO() (gas: 1680031) +KeyperRolesTest:testCannot_ROOT_SAFE_SetRole_ROOT_SAFE_to_EOA_DifferentTree_Safe() (gas: 3012977) +KeyperRolesTest:testCannot_ROOT_SAFE_SetRole_SUPER_SAFE_to_EAO() (gas: 1679916) +KeyperRolesTest:testCannot_ROOT_SAFE_SetRole_SUPER_SAFE_to_SAFE() (gas: 2303128) +KeyperRolesTest:testCannot_SUPER_SAFE_SetRole_SAFE_LEAD_to_EAO() (gas: 1679368) +ModifySafeOwners:testCan_AddOwnerWithThreshold_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2435178) +ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_EOA_is_TARGETS_LEAD() (gas: 1839151) +ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_SAFE_is_TARGETS_LEAD() (gas: 3254480) +ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 3181314) +ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 3254758) +ModifySafeOwners:testCan_AddOwnerWithThreshold_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 2446090) +ModifySafeOwners:testCan_RemoveOwner_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2391200) +ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_EOA_is_TARGETS_LEAD() (gas: 1806494) +ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_SAFE_is_TARGETS_LEAD() (gas: 3214633) +ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 1811083) +ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 2542711) +ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD_DifferentTree() (gas: 3196077) +ModifySafeOwners:testCan_RemoveOwner_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 2402030) +ModifySafeOwners:testRevertInvalidThresholdAddOwnerWithThreshold() (gas: 1878757) +ModifySafeOwners:testRevertInvalidThresholdRemoveOwner() (gas: 1850062) +ModifySafeOwners:testRevertOwnerAlreadyExistsAddOwnerWithThreshold() (gas: 1794906) ModifySafeOwners:testRevertOwnerNotFoundRemoveOwner() (gas: 496767) -ModifySafeOwners:testRevertRootSafeToAttemptTo_AddOwnerWithThreshold_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2948684) -ModifySafeOwners:testRevertRootSafeToAttemptTo_AddOwnerWithThreshold_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 4377455) -ModifySafeOwners:testRevertRootSafeToAttemptTo_removeOwner_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2950193) -ModifySafeOwners:testRevertRootSafeToAttemptTo_removeOwner_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 4378535) -ModifySafeOwners:testRevertRootSafesAttemptToAddToExternalSafeOrg() (gas: 2937582) -ModifySafeOwners:testRevertRootSafesToAttemptToRemoveFromExternalOrg() (gas: 2951390) +ModifySafeOwners:testRevertRootSafeToAttemptTo_AddOwnerWithThreshold_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 3043215) +ModifySafeOwners:testRevertRootSafeToAttemptTo_AddOwnerWithThreshold_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 4471986) +ModifySafeOwners:testRevertRootSafeToAttemptTo_removeOwner_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 3044724) +ModifySafeOwners:testRevertRootSafeToAttemptTo_removeOwner_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 4473066) +ModifySafeOwners:testRevertRootSafesAttemptToAddToExternalSafeOrg() (gas: 3032113) +ModifySafeOwners:testRevertRootSafesToAttemptToRemoveFromExternalOrg() (gas: 3045921) ModifySafeOwners:testRevertSafeNotRegisteredAddOwnerWithThreshold_EOA_Caller() (gas: 936337) ModifySafeOwners:testRevertSafeNotRegisteredAddOwnerWithThreshold_SAFE_Caller() (gas: 940203) ModifySafeOwners:testRevertSafeNotRegisteredRemoveOwner_EOA_Caller() (gas: 505264) ModifySafeOwners:testRevertSafeNotRegisteredRemoveOwner_SAFE_Caller() (gas: 509494) -ModifySafeOwners:testRevertZeroAddressAddOwnerWithThreshold() (gas: 1578192) -ModifySafeOwners:testRevertZeroAddressProvidedRemoveOwner() (gas: 2985563) -TestDeploy:testDeploy() (gas: 6257319) +ModifySafeOwners:testRevertZeroAddressAddOwnerWithThreshold() (gas: 1672722) +ModifySafeOwners:testRevertZeroAddressProvidedRemoveOwner() (gas: 3080094) +TestDeploy:testDeploy() (gas: 6283203) TestDeploySafe:testTransferFundsSafe() (gas: 109672) TestEnableGuard:testEnableKeyperGuard() (gas: 120880) TestEnableGuard:testEnableKeyperModule() (gas: 122295) TestEnableModule:testEnableKeyperModule() (gas: 122305) TestEnableModule:testNewSafeWithKeyperModule() (gas: 470263) TestKeyperSafe:testAuthorityAddress() (gas: 10869) -TestKeyperSafe:testCan_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree() (gas: 2929821) -TestKeyperSafe:testCan_RemoveSquad_SUPER_SAFE_as_SAFE_is_SUPER_SAFE_SameTree() (gas: 2314337) -TestKeyperSafe:testCan_hasNotPermissionOverTarget_is_not_root_of_target() (gas: 1591659) -TestKeyperSafe:testCan_hasNotPermissionOverTarget_is_not_super_safe_of_target() (gas: 2212240) -TestKeyperSafe:testCan_hasNotPermissionOverTarget_is_root_of_target() (gas: 1588959) -TestKeyperSafe:testCan_hasNotPermissionOverTarget_is_super_safe_of_target() (gas: 2207508) -TestKeyperSafe:testCannot_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_DifferentOrg() (gas: 4504891) -TestKeyperSafe:testCannot_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_DifferentTree() (gas: 3000635) -TestKeyperSafe:testCannot_RemoveSquad_SUPER_SAFE_as_SAFE_is_not_TARGET_SUPER_SAFE_DifferentTree() (gas: 4514391) -TestKeyperSafe:testCannot_RemoveSquad_SUPER_SAFE_as_SAFE_is_not_TARGET_SUPER_SAFE_SameTree() (gas: 2854813) -TestKeyperSafe:testDisableDenyHelperList() (gas: 2571020) -TestKeyperSafe:testRemoveSquadAndCheckDisables() (gas: 1692070) +TestKeyperSafe:testCan_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree() (gas: 3025393) +TestKeyperSafe:testCan_RemoveSquad_SUPER_SAFE_as_SAFE_is_SUPER_SAFE_SameTree() (gas: 2411046) +TestKeyperSafe:testCan_hasNotPermissionOverTarget_is_not_root_of_target() (gas: 1686189) +TestKeyperSafe:testCan_hasNotPermissionOverTarget_is_not_super_safe_of_target() (gas: 2308948) +TestKeyperSafe:testCan_hasNotPermissionOverTarget_is_root_of_target() (gas: 1683489) +TestKeyperSafe:testCan_hasNotPermissionOverTarget_is_super_safe_of_target() (gas: 2304216) +TestKeyperSafe:testCannot_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_DifferentOrg() (gas: 4603954) +TestKeyperSafe:testCannot_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_DifferentTree() (gas: 3095166) +TestKeyperSafe:testCannot_RemoveSquad_SUPER_SAFE_as_SAFE_is_not_TARGET_SUPER_SAFE_DifferentTree() (gas: 4613454) +TestKeyperSafe:testCannot_RemoveSquad_SUPER_SAFE_as_SAFE_is_not_TARGET_SUPER_SAFE_SameTree() (gas: 2950385) +TestKeyperSafe:testDisableDenyHelperList() (gas: 2667735) +TestKeyperSafe:testRemoveSquadAndCheckDisables() (gas: 1786600) TestKeyperSafe:testRevertAuthForRegisterOrgTx() (gas: 14097) -TestKeyperSafe:testRevertSuperSafeExecOnBehalfIsDenyList() (gas: 2430770) -TestKeyperSafe:testRevertSuperSafeExecOnBehalfIsNotAllowList() (gas: 2339771) \ No newline at end of file +TestKeyperSafe:testRevertSuperSafeExecOnBehalfIsDenyList() (gas: 2527485) +TestKeyperSafe:testRevertSuperSafeExecOnBehalfIsNotAllowList() (gas: 2436481) \ No newline at end of file diff --git a/Makefile b/Makefile index cfd2e66..6c3a8ed 100644 --- 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 \ No newline at end of file +test-fork-polygon :; source .env && forge script script/SkipExecutionOnBehalf.s.sol:SkipSeveralScenarios --fork-url ${POLYGON_RPC_URL} --private-key ${PRIVATE_KEY} --broadcast -vvvv \ No newline at end of file diff --git a/script/SkipExecutionOnBehalf.s.sol b/script/SkipExecutionOnBehalf.s.sol index 0ca0be1..00fd41c 100644 --- a/script/SkipExecutionOnBehalf.s.sol +++ b/script/SkipExecutionOnBehalf.s.sol @@ -11,19 +11,19 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { 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 +175,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 +207,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 +217,8 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { emptyData, Enum.Operation(0) ); + // ttry 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 +261,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 +278,6 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); // Execute on behalf function - bool result = execTransactionOnBehalfTx( orgHash, rootAddr, @@ -324,7 +321,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 +450,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 +465,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); - // vm.startPrank(callerEOA); - vm.expectRevert("GS020"); + // vm.expectRevert("GS020"); // GS020: Invalid signatures provided keyperModule.execTransactionOnBehalf( orgHash, rootAddr, @@ -510,7 +506,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 +521,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); - // vm.startPrank(callerEOA); - vm.expectRevert("GS026"); + // vm.expectRevert("GS026"); // GS026: Invalid signatures provided keyperModule.execTransactionOnBehalf( orgHash, rootAddr, @@ -584,7 +579,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { ); setGnosisSafe(safeSubSquadA1Addr); - // vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); + // vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); // NotAuthorizedExecOnBehalf: Caller is not authorized to execute on behalf bool result = execTransactionOnBehalfTx( orgHash, safeSubSquadA1Addr, @@ -628,7 +623,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); - // vm.expectRevert("GS013"); + // vm.expectRevert("GS013"); // GS013: Invalid signatures provided execTransactionOnBehalfTx( orgHash, rootAddr, @@ -676,8 +671,8 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { emptyData, Enum.Operation(0) ); - - vm.expectRevert(Errors.InvalidAddressProvided.selector); + // Execute on behalf function with invalid receiver + // vm.expectRevert(Errors.InvalidAddressProvided.selector); // InvalidAddressProvided: Invalid address provided keyperModule.execTransactionOnBehalf( orgHash, rootAddr, @@ -727,12 +722,11 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { ); // Execute on behalf function from a not authorized caller - // vm.startPrank(rootAddr); - vm.expectRevert( - abi.encodeWithSelector( - Errors.InvalidGnosisSafe.selector, address(0) - ) - ); + // vm.expectRevert( + // abi.encodeWithSelector( + // Errors.InvalidGnosisSafe.selector, address(0) + // ) + // ); // InvalidGnosisSafe: Invalid Gnosis Safe keyperModule.execTransactionOnBehalf( orgHash, rootAddr, @@ -781,9 +775,9 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); // Execute on behalf function from a not authorized caller - vm.expectRevert( - abi.encodeWithSelector(Errors.OrgNotRegistered.selector, address(0)) - ); + // vm.expectRevert( + // abi.encodeWithSelector(Errors.OrgNotRegistered.selector, address(0)) + // ); // OrgNotRegistered: Org not registered keyperModule.execTransactionOnBehalf( bytes32(0), rootAddr, @@ -827,12 +821,11 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); // Execute on behalf function from a not authorized caller - - vm.expectRevert( - abi.encodeWithSelector( - Errors.InvalidGnosisSafe.selector, fakeTargetSafe - ) - ); + // vm.expectRevert( + // abi.encodeWithSelector( + // Errors.InvalidGnosisSafe.selector, fakeTargetSafe + // ) + // ); // InvalidGnosisSafe: Invalid Gnosis Safe keyperModule.execTransactionOnBehalf( orgHash, rootAddr, diff --git a/test/ExecTransactionOnBehalf.t.sol b/test/ExecTransactionOnBehalf.t.sol index b906db6..628b282 100644 --- a/test/ExecTransactionOnBehalf.t.sol +++ b/test/ExecTransactionOnBehalf.t.sol @@ -19,6 +19,9 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // Caller Info: ROOT_SAFE(role), SAFE(type), rootSafe(hierachie) // TargetSafe Type: Child from same hierachical tree + // rootSafe ----------- + // | | + // safeSquadA1 <-------- function testCan_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree( ) public { (uint256 rootId, uint256 safeSquadA1) = @@ -65,7 +68,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // safeSubSquadA1 <----- function testCan_ExecTransactionOnBehalf_ROOT_SAFE_and_Target_Root_SameTree_2_levels( ) public { - (uint256 rootId,, uint256 safeSubSquadA1Id,) = keyperSafeBuilder + (uint256 rootId,, uint256 safeSubSquadA1Id,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddr = keyperModule.getSquadSafeAddress(rootId); @@ -135,7 +138,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { DataTypes.Role.SAFE_LEAD, safeSquadBAddr, safeSubSubSquadA1Id, true ); vm.stopPrank(); - + // Verify if the safeSquadBAddr have the role of Safe Lead to execute, executionTransactionOnBehalf assertEq( keyperModule.isSuperSafe(safeSquadBId, safeSubSubSquadA1Id), false ); @@ -189,6 +192,9 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { vm.startPrank(rootAddr); keyperModule.setRole(DataTypes.Role.SAFE_LEAD, callerEOA, rootId, true); vm.stopPrank(); + + // Verify if the callerEOA have the role of Safe Lead to execute, executionTransactionOnBehalf + assertEq(keyperModule.isSafeLead(rootId, callerEOA), true); bytes memory emptyData; bytes memory signatures; @@ -198,13 +204,14 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { rootAddr, rootAddr, receiver, - 2 gwei, + 22 gwei, emptyData, Enum.Operation(0), signatures ); assertEq(result, true); - assertEq(receiver.balance, 2 gwei); + assertEq(receiver.balance, 22 gwei); + vm.stopPrank(); } // execTransactionOnBehalf when is Any EOA, passing the signature of owners of the Root/Super Safe of Target Safe @@ -220,8 +227,8 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // safeSubSquadA1 <----- function testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way( ) public { - (uint256 rootId,, uint256 safeSubSquadA1Id, uint256[] memory ownersPK) = - keyperSafeBuilder.setupOrgThreeTiersTree( + (uint256 rootId,, uint256 safeSubSquadA1Id, uint256[] memory ownersPK,) + = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -257,7 +264,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { rootAddr, safeSubSquadA1Addr, receiver, - 25 gwei, + 37 gwei, emptyData, Enum.Operation(0), nonce @@ -281,13 +288,13 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { rootAddr, safeSubSquadA1Addr, receiver, - 25 gwei, + 37 gwei, emptyData, Enum.Operation(0), concatenatedSignatures ); assertEq(result, true); - assertEq(receiver.balance, 25 gwei); + assertEq(receiver.balance, 37 gwei); vm.stopPrank(); } @@ -304,8 +311,8 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // safeSubSquadA1 <----- function testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Inverse_WAY( ) public { - (uint256 rootId,, uint256 safeSubSquadA1Id, uint256[] memory ownersPK) = - keyperSafeBuilder.setupOrgThreeTiersTree( + (uint256 rootId,, uint256 safeSubSquadA1Id, uint256[] memory ownersPK,) + = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -342,7 +349,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { rootAddr, safeSubSquadA1Addr, receiver, - 25 gwei, + 26 gwei, emptyData, Enum.Operation(0), nonce @@ -366,13 +373,13 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { rootAddr, safeSubSquadA1Addr, receiver, - 25 gwei, + 26 gwei, emptyData, Enum.Operation(0), concatenatedSignatures ); assertEq(result, true); - assertEq(receiver.balance, 25 gwei); + assertEq(receiver.balance, 26 gwei); vm.stopPrank(); } @@ -389,7 +396,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // safeSubSquadA1 <----- function testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES( ) public { - (uint256 rootId,, uint256 safeSubSquadA1Id,) = keyperSafeBuilder + (uint256 rootId,, uint256 safeSubSquadA1Id,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddr = keyperModule.getSquadSafeAddress(rootId); @@ -402,7 +409,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // Random wallet instead of a safe (EOA) address callerEOA = address(0xFED); - // Owner of Root Safe sign args + // Root Safe sign args keyperHelper.setGnosisSafe(rootAddr); bytes memory emptyData; bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( @@ -445,8 +452,10 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // // safeSubSquadA1 <------------ function testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree( ) public { - (, uint256 safeSquadA1Id, uint256 safeSubSquadA1Id,) = keyperSafeBuilder - .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); + (, uint256 safeSquadA1Id, uint256 safeSubSquadA1Id,,) = + keyperSafeBuilder.setupOrgThreeTiersTree( + orgName, squadA1Name, subSquadA1Name + ); address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1Id); @@ -494,6 +503,94 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { assertEq(receiver.balance, 2 gwei); } + // execTransactionOnBehalf when is Any EOA, passing the signature of owners of the Super Safe of Target Safe + // Caller: callerEOA + // Caller Type: EOA + // Caller Role: nothing + // Caller Role: SUPER_SAFE of safeSubSquadA1 + // SuperSafe: squadA1Name + // TargerSafe: safeSubSquadA1, same hierachical tree with 2 levels diff + // safeSquadA1 --------- + // | | + // safeSubSquadA1 <----- + function testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way_from_superSafe( + ) public { + ( + , + uint256 squadA1NId, + uint256 safeSubSquadA1Id, + , + uint256[] memory ownersSuperPK + ) = keyperSafeBuilder.setupOrgThreeTiersTree( + orgName, squadA1Name, subSquadA1Name + ); + + address squadA1NAddr = keyperModule.getSquadSafeAddress(squadA1NId); + address safeSubSquadA1Addr = + keyperModule.getSquadSafeAddress(safeSubSquadA1Id); + + vm.deal(squadA1NAddr, 100 gwei); + vm.deal(safeSubSquadA1Addr, 100 gwei); + + // Random wallet instead of a safe (EOA) + address callerEOA = address(0xFED); + + // get safe from squadA1NAddr + GnosisSafe superSafe = GnosisSafe(payable(squadA1NAddr)); + + // get owners of the root safe + address[] memory owners = superSafe.getOwners(); + uint256 threshold = superSafe.getThreshold(); + uint256 nonce = keyperModule.nonce(); + + // Init Valid Owners + initValidOnwers(4); + + bytes memory concatenatedSignatures; + bytes memory emptyData; + + for (uint256 j = 0; j < threshold; j++) { + address currentOwner = owners[j]; + vm.startPrank(currentOwner); + bytes memory keyperTxHashData = keyperModule.encodeTransactionData( + orgHash, + squadA1NAddr, + safeSubSquadA1Addr, + receiver, + 39 gwei, + emptyData, + Enum.Operation(0), + nonce + ); + bytes32 digest = keccak256(keyperTxHashData); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownersSuperPK[j], digest); + // verify signer + address signer = ecrecover(digest, v, r, s); + assertEq(signer, currentOwner); + bytes memory signature = abi.encodePacked(r, s, v); + concatenatedSignatures = + abi.encodePacked(concatenatedSignatures, signature); + vm.stopPrank(); + } + // verify signature length + assertEq(concatenatedSignatures.length, threshold * 65); + + vm.startPrank(callerEOA); + bool result = keyperModule.execTransactionOnBehalf( + orgHash, + squadA1NAddr, + safeSubSquadA1Addr, + receiver, + 39 gwei, + emptyData, + Enum.Operation(0), + concatenatedSignatures + ); + assertEq(result, true); + assertEq(receiver.balance, 39 gwei); + vm.stopPrank(); + } + // // ! ********************** REVERT ******************** // Revert: "GS026" execTransactionOnBehalf when is Any EOA, passing the wrong signature of the Root/Super Safe of Target Safe @@ -509,7 +606,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // safeSubSquadA1 <----- function testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_WRONG_SIGNATURES( ) public { - (uint256 rootId,, uint256 safeSubSquadA1Id,) = keyperSafeBuilder + (uint256 rootId,, uint256 safeSubSquadA1Id,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddr = keyperModule.getSquadSafeAddress(rootId); @@ -522,7 +619,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // Random wallet instead of a safe (EOA) address callerEOA = address(0xFED); - // Owner of Target Safe signed args + // Owner of Target Safe signed args and of Root/Super Safe keyperHelper.setGnosisSafe(safeSubSquadA1Addr); bytes memory emptyData; bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( @@ -562,7 +659,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // safeSubSquadA1 <----- function testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_INVALID_SIGNATURES( ) public { - (uint256 rootId,, uint256 safeSubSquadA1Id,) = keyperSafeBuilder + (uint256 rootId,, uint256 safeSubSquadA1Id,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddr = keyperModule.getSquadSafeAddress(rootId); @@ -578,6 +675,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // Owner of Root Safe sign args keyperHelper.setGnosisSafe(rootAddr); bytes memory emptyData; + // use invalid signatures bytes memory signatures = keyperHelper.encodeInvalidSignaturesKeyperTx( orgHash, rootAddr, @@ -608,8 +706,8 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // // Caller Role: SUPER_SAFE // // TargerSafe: safeSquadA1 // // TargetSafe Type: safe as lead - // // rootSafe - // // | + // // rootSafe + // // | // // safeSquadA1 <---- // // | | // // safeSubSquadA1 --- @@ -717,6 +815,7 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { address rootAddr = keyperModule.getSquadSafeAddress(rootId); address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); + // Fake receiver = Zero address address fakeReceiver = address(0); // Set keyperhelper gnosis safe to org @@ -892,6 +991,96 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { ); } + // Org with a root safe with 3 child levels: A, B, C + // Squad A starts a executeOnBehalf tx for his children B + // -> The calldata for the function is another executeOnBehalfTx for children C + // -> Verify that this wrapped executeOnBehalf tx does not work + // TODO: test this scenario in Live Testnet + function testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_SAFE( + ) public { + (uint256 rootId, uint256 safeSquadA1, uint256 childSquadA1,,) = + keyperSafeBuilder.setupOrgThreeTiersTree( + orgName, squadA1Name, subSquadA1Name + ); + + address rootAddr = keyperModule.getSquadSafeAddress(rootId); + address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); + address childSquadA1Addr = + keyperModule.getSquadSafeAddress(childSquadA1); + + // Send ETH to squad&subsquad + vm.deal(rootAddr, 100 gwei); + vm.deal(safeSquadA1Addr, 100 gwei); + vm.deal(childSquadA1Addr, 100 gwei); + + // Create a child safe for squad A2 + address fakeCaller = gnosisHelper.newKeyperSafe(4, 2); + bool result = gnosisHelper.createAddSquadTx(safeSquadA1, "ChildSquadA2"); + assertEq(result, true); + + // Set Safe Role in Safe Squad A1 over Child Squad A1 + vm.startPrank(rootAddr); + keyperModule.setRole( + DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY, + fakeCaller, + childSquadA1, + true + ); + assertTrue(keyperModule.isSafeLead(childSquadA1, fakeCaller)); + vm.stopPrank(); + + // Set keyperhelper gnosis safe to fakeCaller + keyperHelper.setGnosisSafe(fakeCaller); + bytes memory emptyData; + bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( + orgHash, + fakeCaller, + childSquadA1Addr, + receiver, + 2 gwei, + emptyData, + Enum.Operation(0) + ); + bytes memory signatures2 = keyperHelper.encodeSignaturesKeyperTx( + orgHash, + rootAddr, + rootAddr, + receiver, + 100 gwei, + emptyData, + Enum.Operation(0) + ); + + vm.startPrank(fakeCaller); + bytes memory internalData = abi.encodeWithSignature( + "execTransactionOnBehalf(bytes32,address,address,address,uint256,bytes,uint8,bytes)", + orgHash, + rootAddr, + rootAddr, + receiver, + 100 gwei, + emptyData, + Enum.Operation(0), + signatures2 + ); + vm.stopPrank(); + + // Execute on behalf function from a not authorized caller (Root Safe of Another Tree) over Super Safe and Squad Safe in another Three + gnosisHelper.updateSafeInterface(fakeCaller); + result = gnosisHelper.execTransactionOnBehalfTx( + orgHash, + fakeCaller, + childSquadA1Addr, + receiver, + 2 gwei, + internalData, + Enum.Operation(0), + signatures + ); + assertEq(result, true); + assertEq(receiver.balance, 2 gwei); // indirect verification, because the receiver is 2 gwei and not 102 gwei + } + // // ! ****************** Reentrancy Attack Test to execOnBehalf *************** function testReentrancyAttack() public { diff --git a/test/Hierarchies.t.sol b/test/Hierarchies.t.sol index e53486d..babbb79 100644 --- a/test/Hierarchies.t.sol +++ b/test/Hierarchies.t.sol @@ -106,7 +106,7 @@ contract Hierarchies is DeployHelper { } function testTreeOrgsTreeMember() public { - (uint256 rootId, uint256 squadIdA1, uint256 subSquadIdA1,) = + (uint256 rootId, uint256 squadIdA1, uint256 subSquadIdA1,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -234,7 +234,7 @@ contract Hierarchies is DeployHelper { } function testCreateSquadThreeTiersTree() public { - (uint256 orgRootId, uint256 safeSquadA1Id, uint256 safeSubSquadA1Id,) = + (uint256 orgRootId, uint256 safeSquadA1Id, uint256 safeSubSquadA1Id,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); diff --git a/test/KeyperGuard.t.sol b/test/KeyperGuard.t.sol index 93a152a..af8efac 100644 --- a/test/KeyperGuard.t.sol +++ b/test/KeyperGuard.t.sol @@ -115,7 +115,7 @@ contract KeyperGuardTest is DeployHelper, SigningUtils { function testCannotDisconnectSafe_As_ROOTSAFE_TARGET_ROOTSAFE_SAME_TREE() public { - (uint256 rootId,, uint256 subSquadA1Id,) = keyperSafeBuilder + (uint256 rootId,, uint256 subSquadA1Id,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddr = keyperModule.getSquadSafeAddress(rootId); @@ -211,7 +211,7 @@ contract KeyperGuardTest is DeployHelper, SigningUtils { } function testCannotDisconnectSafe_As_SuperSafe_As_SameTree() public { - (, uint256 squadIdA1, uint256 subSquadA1Id,) = keyperSafeBuilder + (, uint256 squadIdA1, uint256 subSquadA1Id,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address squadA1Addr = keyperModule.getSquadSafeAddress(squadIdA1); @@ -334,7 +334,7 @@ contract KeyperGuardTest is DeployHelper, SigningUtils { } function testDisconnectSafeBeforeToRemoveSquad_One_Level() public { - (uint256 rootId, uint256 squadIdA1,,) = keyperSafeBuilder + (uint256 rootId, uint256 squadIdA1,,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddr = keyperModule.getSquadSafeAddress(rootId); @@ -391,7 +391,7 @@ contract KeyperGuardTest is DeployHelper, SigningUtils { } function testCannotDisconnectSafe_As_SafeLead_As_EOA() public { - (uint256 rootId,, uint256 childSquadA1,) = keyperSafeBuilder + (uint256 rootId,, uint256 childSquadA1,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddr = keyperModule.getSquadSafeAddress(rootId); @@ -428,7 +428,7 @@ contract KeyperGuardTest is DeployHelper, SigningUtils { } function testCannotDisconnectSafe_As_SafeLead_As_SAFE() public { - (uint256 rootId,, uint256 childSquadA1,) = keyperSafeBuilder + (uint256 rootId,, uint256 childSquadA1,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddr = keyperModule.getSquadSafeAddress(rootId); @@ -622,7 +622,7 @@ contract KeyperGuardTest is DeployHelper, SigningUtils { } function testDisconnectSafe_As_ROOTSAFE_TARGET_ROOT_SAFE() public { - (uint256 rootId, uint256 squadA1Id, uint256 childSquadA1,) = + (uint256 rootId, uint256 squadA1Id, uint256 childSquadA1,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -661,7 +661,7 @@ contract KeyperGuardTest is DeployHelper, SigningUtils { // ! **************** List of Promote to Root ******************************* function testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SQUAD_SAFE() public { - (uint256 rootId, uint256 squadA1Id, uint256 childSquadA1,) = + (uint256 rootId, uint256 squadA1Id, uint256 childSquadA1,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -690,7 +690,7 @@ contract KeyperGuardTest is DeployHelper, SigningUtils { } function testCanPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE() public { - (uint256 rootId, uint256 squadA1Id, uint256 childSquadA1,) = + (uint256 rootId, uint256 squadA1Id, uint256 childSquadA1,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); diff --git a/test/KeyperRoles.t.sol b/test/KeyperRoles.t.sol index 56f5fd8..0252ba3 100644 --- a/test/KeyperRoles.t.sol +++ b/test/KeyperRoles.t.sol @@ -135,7 +135,7 @@ contract KeyperRolesTest is DeployHelper { // Caller Info: Role-> ROOT_SAFE, Type -> SAFE, Hierarchy -> Squad, Name -> root // Target Info: Type-> SAFE, Name -> squadA, Hierarchy related to caller -> N/A function testCannot_ROOT_SAFE_SetRole_SUPER_SAFE_to_SAFE() public { - (uint256 rootId, uint256 squadA1Id, uint256 subSquadAId,) = + (uint256 rootId, uint256 squadA1Id, uint256 subSquadAId,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); diff --git a/test/KeyperSafe.t.sol b/test/KeyperSafe.t.sol index 719559c..38771de 100644 --- a/test/KeyperSafe.t.sol +++ b/test/KeyperSafe.t.sol @@ -37,7 +37,7 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { // Caller Info: Role-> SUPER_SAFE, Type -> SAFE, Hierarchy -> Squad, Name -> safeSquadA1 // Target Info: Type-> SAFE, Name -> safeSubSquadA1, Hierarchy related to caller -> NOT_ALLOW_LIST function testRevertSuperSafeExecOnBehalfIsNotAllowList() public { - (uint256 rootId, uint256 squadA1Id, uint256 subSquadA1Id,) = + (uint256 rootId, uint256 squadA1Id, uint256 subSquadA1Id,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -87,7 +87,7 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { // Caller Info: Role-> SUPER_SAFE, Type -> SAFE, Hierarchy -> Squad, Name -> safeSquadA1 // Target Info: Type-> SAFE, Name -> safeSubSquadA1, Hierarchy related to caller -> DENY_LIST function testRevertSuperSafeExecOnBehalfIsDenyList() public { - (uint256 rootId, uint256 squadA1Id, uint256 subSquadA1Id,) = + (uint256 rootId, uint256 squadA1Id, uint256 subSquadA1Id,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -140,7 +140,7 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { // Caller Info: Role-> SUPER_SAFE, Type -> SAFE, Hierarchy -> Squad, Name -> safeSquadA1 // Target Info: Type-> SAFE, Name -> safeSubSquadA1, Hierarchy related to caller -> DENY_LIST function testDisableDenyHelperList() public { - (uint256 rootId, uint256 squadA1Id, uint256 subSquadA1Id,) = + (uint256 rootId, uint256 squadA1Id, uint256 subSquadA1Id,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -288,7 +288,7 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { function testCan_RemoveSquad_SUPER_SAFE_as_SAFE_is_SUPER_SAFE_SameTree() public { - (uint256 rootId, uint256 squadA1Id, uint256 subSquadA1Id,) = + (uint256 rootId, uint256 squadA1Id, uint256 subSquadA1Id,,) = keyperSafeBuilder.setupOrgThreeTiersTree( orgName, squadA1Name, subSquadA1Name ); @@ -418,7 +418,7 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { function testCan_hasNotPermissionOverTarget_is_super_safe_of_target() public { - (, uint256 squadA1Id, uint256 subSquadA1Id,) = keyperSafeBuilder + (, uint256 squadA1Id, uint256 subSquadA1Id,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address squadAddr = keyperModule.getSquadSafeAddress(squadA1Id); @@ -433,7 +433,7 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { function testCan_hasNotPermissionOverTarget_is_not_super_safe_of_target() public { - (, uint256 squadA1Id, uint256 subSquadA1Id,) = keyperSafeBuilder + (, uint256 squadA1Id, uint256 subSquadA1Id,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address squadAddr = keyperModule.getSquadSafeAddress(squadA1Id); diff --git a/test/ModifySafeOwners.t.sol b/test/ModifySafeOwners.t.sol index 98d8c68..c4ac661 100644 --- a/test/ModifySafeOwners.t.sol +++ b/test/ModifySafeOwners.t.sol @@ -96,7 +96,7 @@ contract ModifySafeOwners is DeployHelper, SigningUtils { // Target Info: Name -> childAAddr, Type -> SAFE,Hierarchy related to caller -> SAME_TREE,CHILDREN function testCan_AddOwnerWithThreshold_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE( ) public { - (, uint256 squadIdA1, uint256 childIdA,) = keyperSafeBuilder + (, uint256 squadIdA1, uint256 childIdA,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address squadAAddr = keyperModule.getSquadSafeAddress(squadIdA1); @@ -130,7 +130,7 @@ contract ModifySafeOwners is DeployHelper, SigningUtils { // Target Info: Name -> squadAAddr, Type -> SAFE, Hierarchy related to caller -> SAME_TREE,CHILDREN function testCan_AddOwnerWithThreshold_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE( ) public { - (uint256 rootIdA, uint256 squadIdA1,,) = keyperSafeBuilder + (uint256 rootIdA, uint256 squadIdA1,,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddrA = keyperModule.getSquadSafeAddress(rootIdA); @@ -706,7 +706,7 @@ contract ModifySafeOwners is DeployHelper, SigningUtils { function testCan_RemoveOwner_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() public { - (, uint256 squadIdA1, uint256 childIdA,) = keyperSafeBuilder + (, uint256 squadIdA1, uint256 childIdA,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address squadAAddr = keyperModule.getSquadSafeAddress(squadIdA1); @@ -737,7 +737,7 @@ contract ModifySafeOwners is DeployHelper, SigningUtils { function testCan_RemoveOwner_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() public { - (uint256 rootIdA, uint256 squadIdA1,,) = keyperSafeBuilder + (uint256 rootIdA, uint256 squadIdA1,,,) = keyperSafeBuilder .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); address rootAddrA = keyperModule.getSquadSafeAddress(rootIdA); diff --git a/test/helpers/KeyperSafeBuilder.t.sol b/test/helpers/KeyperSafeBuilder.t.sol index f796a78..1c9bb42 100644 --- a/test/helpers/KeyperSafeBuilder.t.sol +++ b/test/helpers/KeyperSafeBuilder.t.sol @@ -10,7 +10,8 @@ contract KeyperSafeBuilder is Test { KeyperModule public keyperModule; // fixed array of 4 owners - uint256[] ownersPK = new uint256[](4); + uint256[] ownersRootPK = new uint256[](4); + uint256[] ownersSuperPK = new uint256[](4); mapping(string => address) public keyperSafes; @@ -32,19 +33,22 @@ contract KeyperSafeBuilder is Test { ) public returns (uint256 rootId, uint256 squadIdA1) { // Register Org through safe tx address rootAddr; - (rootAddr, ownersPK) = gnosisHelper.newKeyperSafeWithPKOwners(4, 2); + (rootAddr, ownersRootPK) = gnosisHelper.newKeyperSafeWithPKOwners(4, 2); bool result = gnosisHelper.registerOrgTx(orgNameArg); // Get org Id bytes32 orgHash = keyperModule.getOrgHashBySafe(rootAddr); rootId = keyperModule.getSquadIdBySafe(orgHash, rootAddr); - address squadSafe = gnosisHelper.newKeyperSafe(4, 2); + address squadSafeAddr; + (squadSafeAddr, ownersSuperPK) = + gnosisHelper.newKeyperSafeWithPKOwners(4, 2); + // Create squad through safe tx result = gnosisHelper.createAddSquadTx(rootId, squadA1NameArg); - squadIdA1 = keyperModule.getSquadIdBySafe(orgHash, squadSafe); + squadIdA1 = keyperModule.getSquadIdBySafe(orgHash, squadSafeAddr); vm.deal(rootAddr, 100 gwei); - vm.deal(squadSafe, 100 gwei); + vm.deal(squadSafeAddr, 100 gwei); return (rootId, squadIdA1); } @@ -238,6 +242,7 @@ contract KeyperSafeBuilder is Test { uint256 rootId, uint256 squadIdA1, uint256 subSquadIdA1, + uint256[] memory, uint256[] memory ) { @@ -251,7 +256,7 @@ contract KeyperSafeBuilder is Test { bytes32 orgHash = keyperModule.getOrgBySquad(squadIdA1); // Get subsquadA1 Id subSquadIdA1 = keyperModule.getSquadIdBySafe(orgHash, safeSubSquadA1); - return (rootId, squadIdA1, subSquadIdA1, ownersPK); + return (rootId, squadIdA1, subSquadIdA1, ownersRootPK, ownersSuperPK); } // Deploy 4 keyperSafes : following structure @@ -276,7 +281,7 @@ contract KeyperSafeBuilder is Test { uint256 subSubSquadIdA1 ) { - (rootId, squadIdA1, subSquadIdA1,) = setupOrgThreeTiersTree( + (rootId, squadIdA1, subSquadIdA1,,) = setupOrgThreeTiersTree( orgNameArg, squadA1NameArg, subSquadA1NameArg ); From 250c642458126242404ce4bf9939a1bfedcbcd08 Mon Sep 17 00:00:00 2001 From: Alfredo Lopez Date: Wed, 15 May 2024 10:47:21 +0200 Subject: [PATCH 3/4] FIX: several fixed about comments, and eliminate commented code, and useless --- script/DeployKeyperEnv.s.sol | 3 +- script/DeployKeyperRoles.t.sol | 4 +- script/DeployKeyperSafe.t.sol | 3 +- script/DeployLibraries.s.sol | 2 + script/DeployModuleWithMockedSafe.t.sol | 2 + script/SkipExecutionOnBehalf.s.sol | 2 + src/Helpers.sol | 2 +- src/KeyperGuard.sol | 13 - src/ReentrancyAttack.sol | 32 + src/SigningUtils.sol | 7 + test/DenyHelperKeyperModule.t.sol | 2 - test/ExecTransactionOnBehalf.s.old | 1241 ----------------------- test/KeyperSafe.t.sol | 10 + 13 files changed, 62 insertions(+), 1261 deletions(-) delete mode 100644 test/ExecTransactionOnBehalf.s.old diff --git a/script/DeployKeyperEnv.s.sol b/script/DeployKeyperEnv.s.sol index f1b3993..3c18065 100644 --- 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 general@palmeradao.xyz contract DeployKeyperEnv is Script { function run() public { vm.startBroadcast(); diff --git a/script/DeployKeyperRoles.t.sol b/script/DeployKeyperRoles.t.sol index d7710cb..6deb87e 100644 --- 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 general@palmeradao.xyz contract DeployKeyperRoles is Script { function run() public { // Solenv.config(); diff --git a/script/DeployKeyperSafe.t.sol b/script/DeployKeyperSafe.t.sol index 7ce6502..1766e1e 100644 --- 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 general@palmeradao.xyz contract DeployKeyperSafe is Script { function run() public { Solenv.config(); diff --git a/script/DeployLibraries.s.sol b/script/DeployLibraries.s.sol index f679246..9efa9db 100644 --- 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 general@palmeradao.xyz contract DeployLibraries is Script { function run() public { vm.startBroadcast(); diff --git a/script/DeployModuleWithMockedSafe.t.sol b/script/DeployModuleWithMockedSafe.t.sol index f9ee664..0804243 100644 --- 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 general@palmeradao.xyz contract DeployModuleWithMockedSafe is Script { function run() public { Solenv.config(); diff --git a/script/SkipExecutionOnBehalf.s.sol b/script/SkipExecutionOnBehalf.s.sol index 00fd41c..e55d290 100644 --- a/script/SkipExecutionOnBehalf.s.sol +++ b/script/SkipExecutionOnBehalf.s.sol @@ -6,6 +6,8 @@ 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 general@palmeradao.xyz contract SkipSeveralScenarios is Script, SkipSetupEnv { /// Setup the Environment, with run() from SkipSetupEnvGoerli function setUp() public { diff --git a/src/Helpers.sol b/src/Helpers.sol index c2b29a6..ee07d37 100644 --- 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 general@palmeradao.xyz abstract contract Helpers is DenyHelper, SignatureDecoder { using GnosisSafeMath for uint256; diff --git a/src/KeyperGuard.sol b/src/KeyperGuard.sol index dc98902..b63fa9e 100644 --- a/src/KeyperGuard.sol +++ b/src/KeyperGuard.sol @@ -72,19 +72,6 @@ contract KeyperGuard is BaseGuard, Context { revert Errors.NotAuthorizedAsNotSafeLead(); } } - // Permit to enable guard and module on the same tx - // if ( - // ( - // abi.decode( - // StorageAccessible(caller).getStorageAt( - // uint256(Constants.GUARD_STORAGE_SLOT), 2 - // ), - // (address) - // ) == address(this) - // ) && IGnosisSafe(caller).isModuleEnabled(address(keyperModule)) - // ) { - // revert Errors.SafeNotRegistered(caller); - // } } } } diff --git a/src/ReentrancyAttack.sol b/src/ReentrancyAttack.sol index 8a2acdf..dbe40f4 100644 --- a/src/ReentrancyAttack.sol +++ b/src/ReentrancyAttack.sol @@ -7,6 +7,8 @@ import {Errors} from "../libraries/Errors.sol"; import {DataTypes} from "../libraries/DataTypes.sol"; import {Constants} from "../libraries/Constants.sol"; +/// @title Attacker +/// @custom:security-contact general@palmeradao.xyz contract Attacker { bytes32 public orgFromAttacker; address public superSafeFromAttacker; @@ -38,6 +40,16 @@ contract Attacker { } } + /// Function to perform the attack on the target contract, through the execTransactionOnBehalf + /// @param org ID's Organization + /// @param superSafe Safe super address + /// @param targetSafe Safe target address + /// @param to Address to which the transaction is being sent + /// @param value Value (ETH) that is being sent with the transaction + /// @param data Data payload of the transaction + /// @param operation kind of operation (call or delegatecall) + /// @param signatures Packed signatures data (v, r, s) + /// @return result true if transaction was successful. function performAttack( bytes32 org, address superSafe, @@ -55,12 +67,16 @@ contract Attacker { return true; } + /// function to set the owners of the Safe Multisig Wallet + /// @param _owners Array of owners of the Safe Multisig Wallet function setOwners(address[] memory _owners) public { for (uint256 i = 0; i < owners.length; ++i) { owners[i] = _owners[i]; } } + /// function to get the balance of the Safe Multisig Wallet + /// @param _safe Address of the Safe Multisig Wallet function getBalanceFromSafe(address _safe) external view @@ -69,24 +85,40 @@ contract Attacker { return address(_safe).balance; } + /// function to get the balance of the attacker contract + /// @return balance of the attacker contract function getBalanceFromAttacker() external view returns (uint256) { return address(this).balance; } + /// function to get the balance of the attacker contract + /// @param dataHash Hash of the transaction data to sign + /// @param data Data payload of the transaction + /// @param signatures Packed signatures data (v, r, s) function checkSignatures( bytes32 dataHash, bytes memory data, bytes memory signatures ) external view {} + /// function to get the owners of the Safe Multisig Wallet + /// @return Array of owners of the Safe Multisig Wallet function getOwners() public view returns (address[] memory) { return owners; } + /// function to get the threshold of the Safe Multisig Wallet + /// @return threshold of the Safe Multisig Wallet function getThreshold() public pure returns (uint256) { return uint256(1); } + /// function to set the parameters for the attack + /// @param _org ID's Organization + /// @param _superSafe Safe super address + /// @param _targetSafe Safe target address + /// @param _data Data payload of the transaction + /// @param _signatures Packed signatures data (v, r, s) function setParamsForAttack( bytes32 _org, address _superSafe, diff --git a/src/SigningUtils.sol b/src/SigningUtils.sol index 44b93a7..64bfd9c 100644 --- a/src/SigningUtils.sol +++ b/src/SigningUtils.sol @@ -7,6 +7,7 @@ import {Enum} from "@safe-contracts/common/Enum.sol"; /// @title SigningUtils /// @custom:security-contact general@palmeradao.xyz abstract contract SigningUtils { + /// @dev Transaction structure struct Transaction { address to; uint256 value; @@ -44,6 +45,12 @@ abstract contract SigningUtils { return ECDSA.toTypedDataHash(domainSeparator, structHash); } + /** + * @dev Given a transaction, it creates a hash of the transaction that can be signed + * @param domainSeparatorGnosis Hash of the Gnosis Safe domain separator + * @param safeTx Gnosis Safe transaction + * @return Hash of the transaction + */ function createDigestExecTx( bytes32 domainSeparatorGnosis, Transaction memory safeTx diff --git a/test/DenyHelperKeyperModule.t.sol b/test/DenyHelperKeyperModule.t.sol index 997755a..44464a8 100644 --- a/test/DenyHelperKeyperModule.t.sol +++ b/test/DenyHelperKeyperModule.t.sol @@ -15,8 +15,6 @@ contract DenyHelperKeyperModuleTest is DeployHelper { // Initial Deploy Contracts deployAllContracts(60); // Setup of all Safe for Testing - // org1 = gnosisHelper.setupSeveralSafeEnv(30); - // squadA = gnosisHelper.setupSeveralSafeEnv(30); (RootOrgId, squadIdA1) = keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); org1 = keyperModule.getSquadSafeAddress(RootOrgId); diff --git a/test/ExecTransactionOnBehalf.s.old b/test/ExecTransactionOnBehalf.s.old deleted file mode 100644 index f8a874f..0000000 --- a/test/ExecTransactionOnBehalf.s.old +++ /dev/null @@ -1,1241 +0,0 @@ -// SPDX-License-Identifier: LGPL-3.0-only -pragma solidity ^0.8.15; - -import "forge-std/Test.sol"; -import "../src/SigningUtils.sol"; -import "./helpers/ReentrancyAttackHelper.t.sol"; -import "./helpers/DeployHelper.t.sol"; - -contract ExecTransactionOnBehalf is DeployHelper { - function setUp() public { - DeployHelper.deployAllContracts(60); - } - - // ! ********************** ROOT_SAFE ROLE ******************** - - // Caller Info: ROOT_SAFE(role), SAFE(type), rootSafe(hierachie) - // TargetSafe Type: Child from same hierachical tree - function testCan_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree( - ) public { - (uint256 rootId, uint256 safeSquadA1) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - - // Set keyperhelper gnosis safe to org - keyperHelper.setGnosisSafe(rootAddr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - rootAddr, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - // Execute on behalf function - - gnosisHelper.updateSafeInterface(rootAddr); - bool result = gnosisHelper.execTransactionOnBehalfTx( - orgHash, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - assertEq(result, true); - assertEq(receiver.balance, 2 gwei); - } - - // Caller Info: ROOT_SAFE(role), SAFE(type), rootSafe(hierachie) - // TargerSafe: safeSubSquadA1, same hierachical tree with 2 levels diff - // rootSafe ----------- - // | | - // safeSquadA1 | - // | | - // safeSubSquadA1 <----- - function testCan_ExecTransactionOnBehalf_ROOT_SAFE_and_Target_Root_SameTree_2_levels( - ) public { - (uint256 rootId,, uint256 safeSubSquadA1Id) = keyperSafeBuilder - .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSubSquadA1Addr = - keyperModule.getSquadSafeAddress(safeSubSquadA1Id); - - vm.deal(rootAddr, 100 gwei); - vm.deal(safeSubSquadA1Addr, 100 gwei); - - // Set keyperhelper gnosis safe to org - - keyperHelper.setGnosisSafe(rootAddr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - rootAddr, - safeSubSquadA1Addr, - receiver, - 25 gwei, - emptyData, - Enum.Operation(0) - ); - gnosisHelper.updateSafeInterface(rootAddr); - bool result = gnosisHelper.execTransactionOnBehalfTx( - orgHash, - safeSubSquadA1Addr, - receiver, - 25 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - assertEq(result, true); - assertEq(receiver.balance, 25 gwei); - } - - // ! ********************** SAFE_LEAD ROLE ******************** - - // Caller Info: SAFE_LEAD(role), SAFE(type), squadB(hierachie) - // TargerSafe: safeSubSubSquadA1 - // TargetSafe Type: squad (not a child) - // rootSafe - // | | - // safeSquadA1 safeSquadB - // | - // safeSubSquadA1 - // | - // safeSubSubSquadA1 - function testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() - public - { - (uint256 rootId,, uint256 safeSquadBId,, uint256 safeSubSubSquadA1Id) = - keyperSafeBuilder.setUpBaseOrgTree( - orgName, squadA1Name, squadBName, subSquadA1Name, subSubSquadA1Name - ); - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadBAddr = keyperModule.getSquadSafeAddress(safeSquadBId); - address safeSubSubSquadA1Addr = - keyperModule.getSquadSafeAddress(safeSubSubSquadA1Id); - - vm.deal(safeSubSubSquadA1Addr, 100 gwei); - vm.deal(safeSquadBAddr, 100 gwei); - - vm.startPrank(rootAddr); - keyperModule.setRole( - DataTypes.Role.SAFE_LEAD, safeSquadBAddr, safeSubSubSquadA1Id, true - ); - vm.stopPrank(); - - assertEq( - keyperModule.isSuperSafe(safeSquadBId, safeSubSubSquadA1Id), false - ); - keyperHelper.setGnosisSafe(safeSquadBAddr); - - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - safeSquadBAddr, - safeSubSubSquadA1Addr, - receiver, - 12 gwei, - emptyData, - Enum.Operation(0) - ); - - // Execute on behalf function - bool result = gnosisHelper.execTransactionOnBehalfTx( - orgHash, - safeSubSubSquadA1Addr, - receiver, - 12 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - assertEq(result, true); - assertEq(receiver.balance, 12 gwei); - } - - // execTransactionOnBehalf when SafeLead of an Org as EOA - // Caller: callerEOA - // Caller Type: EOA - // Caller Role: SAFE_LEAD of org - // TargerSafe: rootAddr - // TargetSafe Type: rootSafe - function testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() - public - { - (uint256 rootId,) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - keyperHelper.setGnosisSafe(rootAddr); - - // Random wallet instead of a safe (EOA) - address callerEOA = address(0xFED); - - // Set safe_lead role to fake caller - vm.startPrank(rootAddr); - keyperModule.setRole(DataTypes.Role.SAFE_LEAD, callerEOA, rootId, true); - vm.stopPrank(); - bytes memory emptyData; - bytes memory signatures; - - vm.startPrank(callerEOA); - bool result = keyperModule.execTransactionOnBehalf( - orgHash, - rootAddr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - assertEq(result, true); - assertEq(receiver.balance, 2 gwei); - } - - // ! ********************** SUPER_SAFE ROLE ******************** - - // execTransactionOnBehalf - // Caller: safeSquadA1 - // Caller Type: safe - // Caller Role: SUPER_SAFE of safeSubSquadA1 - // TargerSafe: safeSubSquadA1 - // TargetSafe Type: safe - // rootSafe - // | - // safeSquadA1 as superSafe --- - // | | - // safeSubSquadA1 <------------ - function testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree( - ) public { - (, uint256 safeSquadA1Id, uint256 safeSubSquadA1Id) = keyperSafeBuilder - .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); - - address safeSquadA1Addr = - keyperModule.getSquadSafeAddress(safeSquadA1Id); - address safeSubSquadA1Addr = - keyperModule.getSquadSafeAddress(safeSubSquadA1Id); - - // Send ETH to squad&subsquad - vm.deal(safeSquadA1Addr, 100 gwei); - vm.deal(safeSubSquadA1Addr, 100 gwei); - - // Set keyperhelper gnosis safe to safeSquadA1 - keyperHelper.setGnosisSafe(safeSquadA1Addr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - safeSquadA1Addr, - safeSubSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - - /// Verify if the safeSquadA1Addr have the role to execute, executionTransactionOnBehalf - assertEq( - keyperRolesContract.doesUserHaveRole( - safeSquadA1Addr, uint8(DataTypes.Role.SUPER_SAFE) - ), - true - ); - - // Execute on safe tx - gnosisHelper.updateSafeInterface(safeSquadA1Addr); - bool result = gnosisHelper.execTransactionOnBehalfTx( - orgHash, - safeSubSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - assertEq(result, true); - assertEq(receiver.balance, 2 gwei); - } - - // ! ********************** REVERT ******************** - - // Revert NotAuthorizedExecOnBehalf() execTransactionOnBehalf (safeSubSquadA1 is attempting to execute on its superSafe) - // Caller: safeSubSquadA1 - // Caller Type: safe - // Caller Role: SUPER_SAFE - // TargerSafe: safeSquadA1 - // TargetSafe Type: safe as lead - // rootSafe - // | - // safeSquadA1 <---- - // | | - // safeSubSquadA1 --- - // | - // safeSubSubSquadA1 - function testRevertSuperSafeExecOnBehalf() public { - (uint256 rootId, uint256 squadIdA1, uint256 subSquadIdA1,) = - keyperSafeBuilder.setupOrgFourTiersTree( - orgName, squadA1Name, subSquadA1Name, subSubSquadA1Name - ); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(squadIdA1); - address safeSubSquadA1Addr = - keyperModule.getSquadSafeAddress(subSquadIdA1); - - // Send ETH to org&subsquad - vm.deal(rootAddr, 100 gwei); - vm.deal(safeSquadA1Addr, 100 gwei); - - // Set keyperhelper gnosis safe to safeSubSquadA1 - keyperHelper.setGnosisSafe(safeSubSquadA1Addr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - safeSubSquadA1Addr, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - - vm.startPrank(safeSubSquadA1Addr); - vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); - bool result = keyperModule.execTransactionOnBehalf( - orgHash, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - assertEq(result, false); - } - - // Revert "UNAUTHORIZED" execTransactionOnBehalf (Caller is an EOA but he's not the lead (no role provided to EOA)) - // Caller: fakeCaller - // Caller Type: EOA - // Caller Role: N/A (NO ROLE PROVIDED) - // TargerSafe: safeSquadA1 - // TargetSafe Type: safe - function testRevertNotAuthorizedExecTransactionOnBehalfScenarioThree() - public - { - (uint256 rootId, uint256 safeSquadA1) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - keyperHelper.setGnosisSafe(rootAddr); - - // Random wallet instead of a safe (EOA) - address fakeCaller = address(0xFED); - - keyperHelper.setGnosisSafe(rootAddr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - rootAddr, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - - vm.startPrank(fakeCaller); - vm.expectRevert("UNAUTHORIZED"); - keyperModule.execTransactionOnBehalf( - orgHash, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - } - - // Revert "GS026" execTransactionOnBehalf (invalid signatures provided) - // Caller: rootAddr - // Caller Type: rootSafe - // Caller Role: ROOT_SAFE - // TargerSafe: safeSquadA1 - // TargetSafe Type: safe - function testRevertInvalidSignatureExecOnBehalf() public { - (uint256 rootId, uint256 safeSquadA1) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - keyperHelper.setGnosisSafe(rootAddr); - - // Try onbehalf with incorrect signers - keyperHelper.setGnosisSafe(rootAddr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeInvalidSignaturesKeyperTx( - rootAddr, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - - gnosisHelper.updateSafeInterface(rootAddr); - vm.expectRevert("GS013"); - // Execute invalid OnBehalf function - gnosisHelper.execTransactionOnBehalfTx( - orgHash, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - } - - // 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 { - (uint256 rootId, uint256 safeSquadA1) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - address fakeReceiver = address(0); - - // Set keyperhelper gnosis safe to org - keyperHelper.setGnosisSafe(rootAddr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - rootAddr, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - - // Execute on behalf function from a not authorized caller - vm.startPrank(rootAddr); - vm.expectRevert(Errors.InvalidAddressProvided.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - safeSquadA1Addr, - fakeReceiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - } - - // 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 - { - (uint256 rootId, uint256 safeSquadA1) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - - // Set keyperhelper gnosis safe to org - keyperHelper.setGnosisSafe(rootAddr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - rootAddr, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - - // Execute on behalf function from a not authorized caller - vm.startPrank(rootAddr); - vm.expectRevert( - abi.encodeWithSelector( - Errors.InvalidGnosisSafe.selector, address(0) - ) - ); - keyperModule.execTransactionOnBehalf( - orgHash, - address(0), - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - } - - // 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 - { - (uint256 rootId, uint256 safeSquadA1) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - - // Set keyperhelper gnosis safe to org - keyperHelper.setGnosisSafe(rootAddr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - rootAddr, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - // Execute on behalf function from a not authorized caller - vm.startPrank(rootAddr); - vm.expectRevert( - abi.encodeWithSelector(Errors.OrgNotRegistered.selector, address(0)) - ); - keyperModule.execTransactionOnBehalf( - bytes32(0), - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - } - - // 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 { - (uint256 rootId, uint256 safeSquadA1) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - address fakeTargetSafe = address(0xFFE); - - // Set keyperhelper gnosis safe to org - keyperHelper.setGnosisSafe(rootAddr); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - rootAddr, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - // Execute on behalf function from a not authorized caller - vm.startPrank(rootAddr); - - vm.expectRevert( - abi.encodeWithSelector( - Errors.InvalidGnosisSafe.selector, fakeTargetSafe - ) - ); - keyperModule.execTransactionOnBehalf( - orgHash, - fakeTargetSafe, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - } - - // Revert NotAuthorizedAsNotSafeLead() execTransactionOnBehalf : safe lead of another org/squad - // Caller: fakeCaller - // Caller Type: Safe - // Caller Role: SAFE_LEAD of the org - // TargerSafe: safeSquadA1 - // TargetSafe Type: safe - function testCannot_ExecTransactionOnBehalf_SAFE_LEAD_as_SAFE_Different_Target( - ) public { - (uint256 rootId, uint256 safeSquadA1) = - keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - keyperHelper.setGnosisSafe(rootAddr); - address fakeCaller = gnosisHelper.newKeyperSafe(4, 2); - - // Random wallet instead of a safe (EOA) - - vm.startPrank(fakeCaller); - bool result = gnosisHelper.createAddSquadTx(safeSquadA1, "fakeSquad"); - vm.stopPrank(); - assertEq(result, true); - - // Set keyperhelper gnosis safe to org - bytes memory emptyData; - bytes memory signatures; - - vm.startPrank(rootAddr); - keyperModule.setRole(DataTypes.Role.SAFE_LEAD, fakeCaller, rootId, true); - vm.stopPrank(); - - //Vefiry that fakeCaller is a safe lead - assertEq(keyperModule.isSafeLead(rootId, fakeCaller), true); - - vm.startPrank(fakeCaller); - vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - safeSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - } - - // testCannot_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_DifferentTree - // -> SUPER_SAFE ROLE, caller try call function from another tree - // Revert NotAuthorizedAsNotRootOrSuperSafe() execTransactionOnBehalf : Super Safe in another tree - // Deploy 1 org with 2 root safe with 1 squad each, 1 subsquad each - // RootA RootB - // | | - // squadA1 ---┐ squadB1 - // | │ | - // ChildSquadA └--> ChildSquadB - // Caller: Fake Caller (Super Safe) - // Caller Type: Safe - // Caller Role: Super Safe of the org in another three - // TargerSafe: safeSubSquadB1 - // TargetSafe Type: safe - function testCannot_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_DifferentTree( - ) public { - (, uint256 safeSquadA1,,,, uint256 ChildIdB) = keyperSafeBuilder - .setupTwoRootOrgWithOneSquadAndOneChildEach( - orgName, - squadA1Name, - root2Name, - squadBName, - subSquadA1Name, - "subSquadB1" - ); - - // Inthis case the Fake Caller is a Super Safe of the org in another tree - address fakeCaller = keyperModule.getSquadSafeAddress(safeSquadA1); - address ChildB = keyperModule.getSquadSafeAddress(ChildIdB); - - // Set keyperhelper gnosis safe to org - bytes memory emptyData; - bytes memory signatures; - - // Execute on behalf function from a not authorized caller - vm.startPrank(fakeCaller); - vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - ChildB, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - vm.stopPrank(); - } - - // 3: testCannot_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_DifferentTree - // --> ROOTSAFE from another tree try call function - // Revert NotAuthorizedAsNotRootOrSuperSafe() execTransactionOnBehalf : Root Safe in another tree - // Deploy 1 org with 2 root safe with 1 squad each, 1 subsquad each - // RootA ---┐ RootB - // | │ | - // squadA1 ├----> squadB1 - // | │ | - // ChildSquadA └--> ChildSquadB - // Caller: Fake Caller (Root Safe) - // Caller Type: Safe - // Caller Role: Super Safe of the org in another three - // TargerSafe: safeSubSquadB1 - // TargetSafe Type: safe - function testCannot_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_DifferentTree( - ) public { - (uint256 rootIdA,,, uint256 safeSquadB1,, uint256 ChildIdB) = - keyperSafeBuilder.setupTwoRootOrgWithOneSquadAndOneChildEach( - orgName, - squadA1Name, - root2Name, - squadBName, - subSquadA1Name, - "subSquadB1" - ); - - // Inthis case the Fake Caller is a Root Safe of the org in another tree - address fakeCaller = keyperModule.getSquadSafeAddress(rootIdA); - address safeSquadAddrB1 = keyperModule.getSquadSafeAddress(safeSquadB1); - address ChildB = keyperModule.getSquadSafeAddress(ChildIdB); - - // Set keyperhelper gnosis safe to org - bytes memory emptyData; - bytes memory signatures; - - // Execute on behalf function from a not authorized caller (Root Safe of Another Tree) over Super Safe and Squad Safe in another Three - vm.startPrank(fakeCaller); - vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - safeSquadAddrB1, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - vm.stopPrank(); - - vm.startPrank(fakeCaller); - vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - ChildB, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - vm.stopPrank(); - } - - // testCannot_ExecTransactionOnBehalf_SAFE_LEAD_as_SAFE_Different_Target - // --> SAFE_LEAD from another squad try call function - // Revert NotAuthorizedAsNotRootOrSuperSafe() execTransactionOnBehalf : Safe Lead in another tree - // Deploy 1 org with 2 root safe with 1 squad each, 1 subsquad each - // RootA RootB - // | | - // squadA1 ---┬----> squadB1 - // | │ | - // ChildSquadA └--> ChildSquadB - // Caller: Fake Caller (Super Safe) - // Caller Type: Safe - // Caller Role: Super Safe of the org in another three - // TargerSafe: safeSubSquadB1 - // TargetSafe Type: safe - function testCannot_ExecTransactionOnBehalf_SAFE_LEAD_as_SAFE_Different_Tree( - ) public { - ( - uint256 rootIdA, - uint256 safeSquadA1, - , - uint256 safeSquadB1, - uint256 ChildIdA, - uint256 ChildIdB - ) = keyperSafeBuilder.setupTwoRootOrgWithOneSquadAndOneChildEach( - orgName, - squadA1Name, - root2Name, - squadBName, - subSquadA1Name, - "subSquadB1" - ); - - // Inthis case the Fake Caller is a Super Safe of the org in another tree - address rootAddrA = keyperModule.getSquadSafeAddress(rootIdA); - address fakeCaller = keyperModule.getSquadSafeAddress(safeSquadA1); - address safeSquadAddrB1 = keyperModule.getSquadSafeAddress(safeSquadB1); - address ChildA1 = keyperModule.getSquadSafeAddress(ChildIdA); - address ChildB = keyperModule.getSquadSafeAddress(ChildIdB); - - // Set keyperhelper gnosis safe to org - bytes memory emptyData; - bytes memory signatures; - - // Set Safe Role in Safe Squad A1 over Child Squad A1 - vm.startPrank(rootAddrA); - uint256 childSquadA1 = keyperModule.getSquadIdBySafe(orgHash, ChildA1); - keyperModule.setRole( - DataTypes.Role.SAFE_LEAD, fakeCaller, childSquadA1, true - ); - assertTrue(keyperModule.isSafeLead(childSquadA1, fakeCaller)); - vm.stopPrank(); - - // Execute on behalf function from a not authorized caller (Root Safe of Another Tree) over Super Safe and Squad Safe in another Three - vm.startPrank(fakeCaller); - vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - safeSquadAddrB1, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - - vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - ChildB, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - vm.stopPrank(); - } - - // testCannot_ExecTransactionOnBehalf_SAFE_LEAD_as_EOA_Different_Target - // --> SAFE_LEAD from another squad try call function - // Revert NotAuthorizedAsNotRootOrSuperSafe() execTransactionOnBehalf : Safe Lead in another tree by EOA as caller - // Deploy 1 org with 2 root safe with 1 squad each, 1 subsquad each - // RootA RootB - // | | - // ┌---Tx------> squadA1 ---┬-----> squadB1 - // │ | │ | - // EOA Lead of->ChildSquadA └---> ChildSquadB - // Caller: Fake Caller (EOA Safe Lead) - // Caller Type: Safe - // Caller Role: Super Safe of the org in another three - // TargerSafe: safeSubSquadB1 - // TargetSafe Type: safe - function testCannot_ExecTransactionOnBehalf_SAFE_LEAD_as_EOA_Different_Target( - ) public { - ( - uint256 rootIdA, - uint256 safeSquadA1, - , - uint256 safeSquadB1, - uint256 ChildIdA, - uint256 ChildIdB - ) = keyperSafeBuilder.setupTwoRootOrgWithOneSquadAndOneChildEach( - orgName, - squadA1Name, - root2Name, - squadBName, - subSquadA1Name, - "subSquadB1" - ); - - // Inthis case the Fake Caller is a Super Safe of the org in another tree - address rootAddrA = keyperModule.getSquadSafeAddress(rootIdA); - address safeSquadAddrA1 = keyperModule.getSquadSafeAddress(safeSquadA1); - address safeSquadAddrB1 = keyperModule.getSquadSafeAddress(safeSquadB1); - address ChildA1 = keyperModule.getSquadSafeAddress(ChildIdA); - address ChildB = keyperModule.getSquadSafeAddress(ChildIdB); - - // Set keyperhelper gnosis safe to org - bytes memory emptyData; - bytes memory signatures; - - // Random wallet instead of a safe (EOA) - address fakeCaller = address(0xFED); - - // Set Safe Role in Safe Squad A1 over Child Squad A1 - vm.startPrank(rootAddrA); - uint256 childSquadA1 = keyperModule.getSquadIdBySafe(orgHash, ChildA1); - keyperModule.setRole( - DataTypes.Role.SAFE_LEAD, fakeCaller, childSquadA1, true - ); - assertTrue(keyperModule.isSafeLead(childSquadA1, fakeCaller)); - vm.stopPrank(); - - // Execute on behalf function from a not authorized caller (EOA) over another Squad - vm.startPrank(fakeCaller); - vm.expectRevert(Errors.NotAuthorizedAsNotSafeLead.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - safeSquadAddrB1, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - vm.expectRevert(Errors.NotAuthorizedAsNotSafeLead.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - ChildB, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - vm.expectRevert(Errors.NotAuthorizedAsNotSafeLead.selector); - keyperModule.execTransactionOnBehalf( - orgHash, - safeSquadAddrA1, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - vm.stopPrank(); - } - - // testCan_ExecTransactionOnBehalf_SAFE_LEAD_EXEC_ON_BEHALF_ONLY_as_SAFE_is_TARGETS_LEAD - // -> SAFE_LEAD_EXEC_ON_BEHALF_ONLY to target squad which caller is lead - // Deploy 3 keyperSafes : following structure - // RootOrg - // | - // safeSquadA1 ----------------------------------┐ - // | │ - // ChildSquadA1 <-SAFE Lead Exec on Behalf--- ChildSquadA2 - // Caller: Right Caller (ChildSquadA2 Safe Lead of childSquadA1) - // Caller Type: Safe - // Caller Role: Squad of the org in the same tree - // TargerSafe: ChildSquadA1 - // TargetSafe Type: safe - function testCan_ExecTransactionOnBehalf_SAFE_LEAD_EXEC_ON_BEHALF_ONLY_as_SAFE_is_TARGETS_LEAD( - ) public { - (uint256 rootId, uint256 safeSquadA1, uint256 childSquadA1) = - keyperSafeBuilder.setupOrgThreeTiersTree( - orgName, squadA1Name, subSquadA1Name - ); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address childSquadA1Addr = - keyperModule.getSquadSafeAddress(childSquadA1); - - // Send ETH to squad&subsquad - vm.deal(rootAddr, 100 gwei); - vm.deal(childSquadA1Addr, 100 gwei); - - // Create a child safe for squad A2 - address rightCaller = gnosisHelper.newKeyperSafe(4, 2); - bool result = gnosisHelper.createAddSquadTx(safeSquadA1, "ChildSquadA2"); - assertEq(result, true); - - // Set keyperhelper gnosis safe to rightCaller - keyperHelper.setGnosisSafe(rightCaller); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - rightCaller, - childSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - - // Set Safe Role in Safe Squad A1 over Child Squad A1 - vm.startPrank(rootAddr); - keyperModule.setRole( - DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY, - rightCaller, - childSquadA1, - true - ); - assertTrue(keyperModule.isSafeLead(childSquadA1, rightCaller)); - vm.stopPrank(); - - // Execute on behalf function from a not authorized caller (Root Safe of Another Tree) over Super Safe and Squad Safe in another Three - gnosisHelper.updateSafeInterface(rightCaller); - - gnosisHelper.execTransactionOnBehalfTx( - orgHash, - childSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - assertEq(receiver.balance, 2 gwei); - } - - // testCan_ExecTransactionOnBehalf_SAFE_LEAD_EXEC_ON_BEHALF_ONLY_as_EOA_is_TARGETS_LEAD - // -> SAFE_LEAD_EXEC_ON_BEHALF_ONLY (EOA) to target squad which caller is lead - // Deploy 3 keyperSafes : following structure - // RootOrg - // | - // safeSquadA1 - // | - // ChildSquadA1 <-SAFE Lead Exec on Behalf--- EOA Caller - // Caller: Right Caller (ChildSquadA2 Safe Lead of childSquadA1) - // Caller Type: Safe - // Caller Role: Squad of the org in the same tree - // TargerSafe: ChildSquadA1 - // TargetSafe Type: safe - function testCan_ExecTransactionOnBehalf_SAFE_LEAD_EXEC_ON_BEHALF_ONLY_as_EOA_is_TARGETS_LEAD( - ) public { - (uint256 rootId,, uint256 childSquadA1) = keyperSafeBuilder - .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address childSquadA1Addr = - keyperModule.getSquadSafeAddress(childSquadA1); - - // Send ETH to squad&subsquad - vm.deal(rootAddr, 100 gwei); - vm.deal(childSquadA1Addr, 100 gwei); - - // Create a a Ramdom Right EOA Caller - address rightCaller = address(0xCBA); - - // Set keyperhelper gnosis safe to org - bytes memory emptyData; - bytes memory signatures; - - // Set Safe Role in Safe Squad A1 over Child Squad A1 - vm.startPrank(rootAddr); - keyperModule.setRole( - DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY, - rightCaller, - childSquadA1, - true - ); - assertTrue(keyperModule.isSafeLead(childSquadA1, rightCaller)); - vm.stopPrank(); - - // Execute on behalf function from a not authorized caller (Root Safe of Another Tree) over Super Safe and Squad Safe in another Three - vm.startPrank(rightCaller); - - keyperModule.execTransactionOnBehalf( - orgHash, - childSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - assertEq(receiver.balance, 2 gwei); - vm.stopPrank(); - } - - // Org with a root safe with 3 child levels: A, B, C - // Squad A starts a executeOnBehalf tx for his children B - // -> The calldata for the function is another executeOnBehalfTx for children C - // -> Verify that this wrapped executeOnBehalf tx does not work - // TODO: test this scenario in Live Testnet - function testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_SAFE( - ) public { - (uint256 rootId, uint256 safeSquadA1, uint256 childSquadA1) = - keyperSafeBuilder.setupOrgThreeTiersTree( - orgName, squadA1Name, subSquadA1Name - ); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address safeSquadA1Addr = keyperModule.getSquadSafeAddress(safeSquadA1); - address childSquadA1Addr = - keyperModule.getSquadSafeAddress(childSquadA1); - - // Send ETH to squad&subsquad - vm.deal(rootAddr, 100 gwei); - vm.deal(safeSquadA1Addr, 100 gwei); - vm.deal(childSquadA1Addr, 100 gwei); - - // Create a child safe for squad A2 - address fakeCaller = gnosisHelper.newKeyperSafe(4, 2); - bool result = gnosisHelper.createAddSquadTx(safeSquadA1, "ChildSquadA2"); - assertEq(result, true); - - // Set Safe Role in Safe Squad A1 over Child Squad A1 - vm.startPrank(rootAddr); - keyperModule.setRole( - DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY, - fakeCaller, - childSquadA1, - true - ); - assertTrue(keyperModule.isSafeLead(childSquadA1, fakeCaller)); - vm.stopPrank(); - - // Set keyperhelper gnosis safe to fakeCaller - keyperHelper.setGnosisSafe(fakeCaller); - bytes memory emptyData; - bytes memory signatures = keyperHelper.encodeSignaturesKeyperTx( - fakeCaller, - childSquadA1Addr, - receiver, - 2 gwei, - emptyData, - Enum.Operation(0) - ); - bytes memory signatures2 = keyperHelper.encodeSignaturesKeyperTx( - fakeCaller, - rootAddr, - receiver, - 100 gwei, - emptyData, - Enum.Operation(0) - ); - - bytes memory internalData = abi.encodeWithSignature( - "execTransactionOnBehalf(bytes32,address,address,uint256,bytes,uint8,bytes)", - orgHash, - rootAddr, - receiver, - 100 gwei, - emptyData, - Enum.Operation(0), - signatures2 - ); - - // Execute on behalf function from a not authorized caller (Root Safe of Another Tree) over Super Safe and Squad Safe in another Three - gnosisHelper.updateSafeInterface(fakeCaller); - vm.expectRevert("GS013"); - result = gnosisHelper.execTransactionOnBehalfTx( - orgHash, - childSquadA1Addr, - receiver, - 2 gwei, - internalData, - Enum.Operation(0), - signatures - ); - assertEq(result, false); - assertEq(receiver.balance, 0 gwei); - } - - // Org with a root safe with 3 child levels: A, B, C - // Squad A starts a executeOnBehalf tx for his children B - // -> The calldata for the function is another executeOnBehalfTx for children C - // -> Verify that this wrapped executeOnBehalf tx does not work - // TODO: test this scenario in Live Testnet - function testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_EOA( - ) public { - (uint256 rootId,, uint256 childSquadA1) = keyperSafeBuilder - .setupOrgThreeTiersTree(orgName, squadA1Name, subSquadA1Name); - - address rootAddr = keyperModule.getSquadSafeAddress(rootId); - address childSquadA1Addr = - keyperModule.getSquadSafeAddress(childSquadA1); - - // Send ETH to squad&subsquad - vm.deal(rootAddr, 100 gwei); - vm.deal(childSquadA1Addr, 100 gwei); - - // Create a fakeCaller with Ramdom EOA - address fakeCaller = address(0xCBA); - - // Set Safe Role in Safe Squad A1 over Child Squad A1 - vm.startPrank(rootAddr); - keyperModule.setRole( - DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY, - fakeCaller, - childSquadA1, - true - ); - assertTrue(keyperModule.isSafeLead(childSquadA1, fakeCaller)); - vm.stopPrank(); - - // Value to be sent to the receiver with rightCaller - bytes memory emptyData; - bytes memory signatures; - // Set keyperhelper gnosis safe to rootAddr - keyperHelper.setGnosisSafe(childSquadA1Addr); - bytes memory signatures2 = keyperHelper.encodeSignaturesKeyperTx( - childSquadA1Addr, - rootAddr, - receiver, - 100 gwei, - emptyData, - Enum.Operation(0) - ); - - bytes memory internalData = abi.encodeWithSignature( - "execTransactionOnBehalf(bytes32,address,address,uint256,bytes,uint8,bytes)", - orgHash, - rootAddr, - receiver, - 100 gwei, - emptyData, - Enum.Operation(0), - signatures2 - ); - - // Execute on behalf function from a not authorized caller (Root Safe of Another Tree) over Super Safe and Squad Safe in another Three - vm.startPrank(fakeCaller); - bool result = keyperModule.execTransactionOnBehalf( - orgHash, - childSquadA1Addr, - receiver, - 50 gwei, - internalData, - Enum.Operation(0), - signatures - ); - vm.stopPrank(); - assertTrue(result); - assertEq(receiver.balance, 50 gwei); // Indirect Validattion - } - - // ! ****************** Reentrancy Attack Test to execOnBehalf *************** - - function testReentrancyAttack() public { - Attacker attackerContract = new Attacker(address(keyperModule)); - AttackerHelper attackerHelper = new AttackerHelper(); - attackerHelper.initHelper( - keyperModule, attackerContract, gnosisHelper, 30 - ); - - (, address attacker, address victim) = - attackerHelper.setAttackerTree(orgName); - - gnosisHelper.updateSafeInterface(victim); - attackerContract.setOwners(gnosisHelper.gnosisSafe().getOwners()); - - gnosisHelper.updateSafeInterface(attacker); - vm.startPrank(attacker); - - bytes memory emptyData; - bytes memory signatures = attackerHelper - .encodeSignaturesForAttackKeyperTx( - attacker, victim, attacker, 5 gwei, emptyData, Enum.Operation(0) - ); - - vm.expectRevert(Errors.TxOnBehalfExecutedFailed.selector); - bool result = attackerContract.performAttack( - orgHash, - victim, - attacker, - 5 gwei, - emptyData, - Enum.Operation(0), - signatures - ); - - assertEq(result, false); - - // This is the expected behavior since the nonReentrant modifier is blocking the attacker from draining the victim's funds nor transfer any amount - assertEq(attackerContract.getBalanceFromSafe(victim), 100 gwei); - assertEq(attackerContract.getBalanceFromAttacker(), 0); - } -} diff --git a/test/KeyperSafe.t.sol b/test/KeyperSafe.t.sol index 38771de..1dbb6ba 100644 --- a/test/KeyperSafe.t.sol +++ b/test/KeyperSafe.t.sol @@ -259,6 +259,8 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { keyperModule.removeSquad(squadAId); } + // Caller Info: Role-> ROOT_SAFE, Type -> SAFE, Hierarchy -> Root, Name -> rootA + // Target Info: Type-> ROOT_SAFE, Name -> rootB, Hierarchy related to caller -> DIFFERENT_TREE function testCannot_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_DifferentOrg( ) public { (uint256 rootIdA, uint256 squadAId, uint256 rootIdB, uint256 squadBId,,) @@ -387,6 +389,8 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { ); } + // Caller Info: Role-> ROOT_SAFE, Type -> SAFE, Hierarchy -> Root, Name -> rootA + // Target Info: Type-> SUPER_SAFE, Name -> squadA, Hierarchy related to caller -> SAME_TREE function testCan_hasNotPermissionOverTarget_is_root_of_target() public { (uint256 rootId, uint256 squadA1Id) = keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); @@ -400,6 +404,8 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { assertFalse(result); } + // Caller Info: Role-> SUPER_SAFE, Type -> SAFE, Hierarchy -> Super, Name -> squadA + // Target Info: Type-> ROOT_SAFE, Name -> rootA, Hierarchy related to caller -> SAME_TREE function testCan_hasNotPermissionOverTarget_is_not_root_of_target() public { @@ -415,6 +421,8 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { assertTrue(result); } + // Caller Info: Role-> SUPER_SAFE, Type -> SAFE, Hierarchy -> Super, Name -> squadA + // Target Info: Type-> CHILD_SAFE, Name -> subSquadA, Hierarchy related to caller -> SAME_TREE function testCan_hasNotPermissionOverTarget_is_super_safe_of_target() public { @@ -430,6 +438,8 @@ contract TestKeyperSafe is SigningUtils, DeployHelper { assertFalse(result); } + // Caller Info: Role-> CHILD_SAFE, Type -> SAFE, Hierarchy -> Child, Name -> subSquadA + // Target Info: Type-> SUPER_SAFE, Name -> squadA, Hierarchy related to caller -> SAME_TREE function testCan_hasNotPermissionOverTarget_is_not_super_safe_of_target() public { From 4e54e9ef1c390e2693baddc665dd6cc7e5bd76b5 Mon Sep 17 00:00:00 2001 From: Alfredo Lopez Date: Tue, 21 May 2024 20:55:29 +0200 Subject: [PATCH 4/4] FIX: fix all comments @jozer-rami --- .gas-snapshot | 2 +- script/SkipExecutionOnBehalf.s.sol | 102 +++++++++++------------- test/ExecTransactionOnBehalf.t.sol | 121 ++++++++++++++--------------- 3 files changed, 105 insertions(+), 120 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index a244163..986b0a2 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -135,7 +135,7 @@ ModifySafeOwners:testRevertSafeNotRegisteredRemoveOwner_EOA_Caller() (gas: 50526 ModifySafeOwners:testRevertSafeNotRegisteredRemoveOwner_SAFE_Caller() (gas: 509494) ModifySafeOwners:testRevertZeroAddressAddOwnerWithThreshold() (gas: 1672722) ModifySafeOwners:testRevertZeroAddressProvidedRemoveOwner() (gas: 3080094) -TestDeploy:testDeploy() (gas: 6283203) +TestDeploy:testDeploy() (gas: 6285029) TestDeploySafe:testTransferFundsSafe() (gas: 109672) TestEnableGuard:testEnableKeyperGuard() (gas: 120880) TestEnableGuard:testEnableKeyperModule() (gas: 122295) diff --git a/script/SkipExecutionOnBehalf.s.sol b/script/SkipExecutionOnBehalf.s.sol index e55d290..bb418b7 100644 --- a/script/SkipExecutionOnBehalf.s.sol +++ b/script/SkipExecutionOnBehalf.s.sol @@ -219,7 +219,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { emptyData, Enum.Operation(0) ); - // ttry to encode the end of the signature, from EOA random the internal data to the Wrapper of + // 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)", @@ -467,7 +467,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); - // vm.expectRevert("GS020"); // GS020: Invalid signatures provided + // GS020: Invalid signatures provided keyperModule.execTransactionOnBehalf( orgHash, rootAddr, @@ -523,7 +523,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); - // vm.expectRevert("GS026"); // GS026: Invalid signatures provided + // GS026: Invalid signatures provided keyperModule.execTransactionOnBehalf( orgHash, rootAddr, @@ -581,7 +581,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { ); setGnosisSafe(safeSubSquadA1Addr); - // vm.expectRevert(Errors.NotAuthorizedExecOnBehalf.selector); // NotAuthorizedExecOnBehalf: Caller is not authorized to execute on behalf + // NotAuthorizedExecOnBehalf: Caller is not authorized to execute on behalf bool result = execTransactionOnBehalfTx( orgHash, safeSubSquadA1Addr, @@ -625,7 +625,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); - // vm.expectRevert("GS013"); // GS013: Invalid signatures provided + // GS013: Invalid signatures provided execTransactionOnBehalfTx( orgHash, rootAddr, @@ -639,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(); @@ -674,7 +674,6 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); // Execute on behalf function with invalid receiver - // vm.expectRevert(Errors.InvalidAddressProvided.selector); // InvalidAddressProvided: Invalid address provided keyperModule.execTransactionOnBehalf( orgHash, rootAddr, @@ -688,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 { @@ -723,12 +722,7 @@ contract SkipSeveralScenarios is Script, SkipSetupEnv { Enum.Operation(0) ); - // Execute on behalf function from a not authorized caller - // vm.expectRevert( - // abi.encodeWithSelector( - // Errors.InvalidGnosisSafe.selector, address(0) - // ) - // ); // InvalidGnosisSafe: Invalid Gnosis Safe + // InvalidGnosisSafe: Invalid Gnosis Safe keyperModule.execTransactionOnBehalf( orgHash, rootAddr, @@ -742,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 { @@ -776,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 + // OrgNotRegistered: Org not registered keyperModule.execTransactionOnBehalf( bytes32(0), rootAddr, @@ -793,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) = @@ -822,12 +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 + // InvalidGnosisSafe: Invalid Gnosis Safe keyperModule.execTransactionOnBehalf( orgHash, rootAddr, diff --git a/test/ExecTransactionOnBehalf.t.sol b/test/ExecTransactionOnBehalf.t.sol index 628b282..535d2ac 100644 --- a/test/ExecTransactionOnBehalf.t.sol +++ b/test/ExecTransactionOnBehalf.t.sol @@ -439,17 +439,17 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // // ! ********************** SUPER_SAFE ROLE ******************** - // // execTransactionOnBehalf - // // Caller: safeSquadA1 - // // Caller Type: safe - // // Caller Role: SUPER_SAFE of safeSubSquadA1 - // // TargerSafe: safeSubSquadA1 - // // TargetSafe Type: safe - // // rootSafe - // // | - // // safeSquadA1 as superSafe --- - // // | | - // // safeSubSquadA1 <------------ + // execTransactionOnBehalf + // Caller: safeSquadA1 + // Caller Type: safe + // Caller Role: SUPER_SAFE of safeSubSquadA1 + // TargerSafe: safeSubSquadA1 + // TargetSafe Type: safe + // rootSafe + // | + // safeSquadA1 as superSafe --- + // | | + // safeSubSquadA1 <------------ function testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree( ) public { (, uint256 safeSquadA1Id, uint256 safeSubSquadA1Id,,) = @@ -700,19 +700,19 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { ); } - // // Revert NotAuthorizedExecOnBehalf() execTransactionOnBehalf (safeSubSquadA1 is attempting to execute on its superSafe) - // // Caller: safeSubSquadA1 - // // Caller Type: safe - // // Caller Role: SUPER_SAFE - // // TargerSafe: safeSquadA1 - // // TargetSafe Type: safe as lead - // // rootSafe - // // | - // // safeSquadA1 <---- - // // | | - // // safeSubSquadA1 --- - // // | - // // safeSubSubSquadA1 + // Revert NotAuthorizedExecOnBehalf() execTransactionOnBehalf (safeSubSquadA1 is attempting to execute on its superSafe) + // Caller: safeSubSquadA1 + // Caller Type: safe + // Caller Role: SUPER_SAFE + // TargerSafe: safeSquadA1 + // TargetSafe Type: safe as lead + // rootSafe + // | + // safeSquadA1 <---- + // | | + // safeSubSquadA1 --- + // | + // safeSubSubSquadA1 function testRevertSuperSafeExecOnBehalf() public { (uint256 rootId, uint256 squadIdA1, uint256 subSquadIdA1,) = keyperSafeBuilder.setupOrgFourTiersTree( @@ -798,16 +798,16 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { ); } - // // 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 { (uint256 rootId, uint256 safeSquadA1) = @@ -846,16 +846,16 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { ); } - // // 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 { @@ -897,16 +897,16 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { ); } - // // 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 { @@ -945,12 +945,12 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { ); } - // // 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 { (uint256 rootId, uint256 safeSquadA1) = keyperSafeBuilder.setupRootOrgAndOneSquad(orgName, squadA1Name); @@ -995,7 +995,6 @@ contract ExecTransactionOnBehalf is DeployHelper, SignersHelper { // Squad A starts a executeOnBehalf tx for his children B // -> The calldata for the function is another executeOnBehalfTx for children C // -> Verify that this wrapped executeOnBehalf tx does not work - // TODO: test this scenario in Live Testnet function testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_SAFE( ) public { (uint256 rootId, uint256 safeSquadA1, uint256 childSquadA1,,) =