FIX: fixed possible reentrancy in Helpers._executeModuleTransaction

.gas-snapshot

@@ -17,35 +17,35 @@ DenyHelperPalmeraModuleTest:testRevertInvalidRootSafe() (gas: 404034)
 DenyHelperPalmeraModuleTest:testRevertInvalidSafe() (gas: 187896)
 EventsChekers:testEventWhenAddSquad() (gas: 1334435)
 EventsChekers:testEventWhenAddToList() (gas: 986539)
-EventsChekers:testEventWhenDisconnectSafe() (gas: 1325567)
+EventsChekers:testEventWhenDisconnectSafe() (gas: 1328307)
 EventsChekers:testEventWhenDropFromList() (gas: 993921)
-EventsChekers:testEventWhenExecutionOnBehalf() (gas: 1889477)
+EventsChekers:testEventWhenExecutionOnBehalf() (gas: 1889442)
 EventsChekers:testEventWhenPromoteRootSafe() (gas: 2030557)
 EventsChekers:testEventWhenRegisterRootOrg() (gas: 732317)
 EventsChekers:testEventWhenRegisterRootSafe() (gas: 1341963)
 EventsChekers:testEventWhenRemoveSquad() (gas: 1389703)
-EventsChekers:testEventWhenRemoveWholeTree() (gas: 2518445)
+EventsChekers:testEventWhenRemoveWholeTree() (gas: 2540333)
 EventsChekers:testEventWhenUpdateNewLimit() (gas: 1372640)
 EventsChekers:testEventWhenUpdateSuper() (gas: 2450106)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_ROOT_SAFE_and_Target_Root_SameTree_2_levels() (gas: 2571771)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree() (gas: 1945262)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 1870766)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 3816452)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree() (gas: 2582754)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES() (gas: 2511775)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Inverse_WAY() (gas: 2693625)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way() (gas: 2693469)
-ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way_from_superSafe() (gas: 2696170)
-ExecTransactionOnBehalf:testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_SAFE() (gas: 3304124)
-ExecTransactionOnBehalf:testReentrancyAttack() (gas: 6553031)
-ExecTransactionOnBehalf:testRevertInvalidAddressProvidedExecTransactionOnBehalfScenarioOne() (gas: 1749779)
-ExecTransactionOnBehalf:testRevertInvalidSafeExecTransactionOnBehalf() (gas: 1741316)
-ExecTransactionOnBehalf:testRevertInvalidSignatureExecOnBehalf() (gas: 1863718)
-ExecTransactionOnBehalf:testRevertOrgNotRegisteredExecTransactionOnBehalfScenarioThree() (gas: 1756750)
-ExecTransactionOnBehalf:testRevertSuperSafeExecOnBehalf() (gas: 3030998)
-ExecTransactionOnBehalf:testRevertZeroAddressProvidedExecTransactionOnBehalfScenarioTwo() (gas: 1739039)
-ExecTransactionOnBehalf:testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_INVALID_SIGNATURES() (gas: 2442235)
-ExecTransactionOnBehalf:testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_WRONG_SIGNATURES() (gas: 2425994)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_ROOT_SAFE_and_Target_Root_SameTree_2_levels() (gas: 2571736)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SameTree() (gas: 1945227)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 1870731)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 3816417)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_SUPER_SAFE_as_SAFE_is_TARGETS_LEAD_SameTree() (gas: 2582719)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES() (gas: 2511740)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Inverse_WAY() (gas: 2693590)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way() (gas: 2693434)
+ExecTransactionOnBehalf:testCan_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_RIGHTS_SIGNATURES_signed_one_by_one_Straight_way_from_superSafe() (gas: 2696135)
+ExecTransactionOnBehalf:testCannot_ExecTransactionOnBehalf_Wrapper_ExecTransactionOnBehalf_ChildSquad_over_RootSafe_With_SAFE() (gas: 3304089)
+ExecTransactionOnBehalf:testReentrancyAttack() (gas: 6553008)
+ExecTransactionOnBehalf:testRevertInvalidAddressProvidedExecTransactionOnBehalfScenarioOne() (gas: 1749744)
+ExecTransactionOnBehalf:testRevertInvalidSafeExecTransactionOnBehalf() (gas: 1741281)
+ExecTransactionOnBehalf:testRevertInvalidSignatureExecOnBehalf() (gas: 1863683)
+ExecTransactionOnBehalf:testRevertOrgNotRegisteredExecTransactionOnBehalfScenarioThree() (gas: 1756715)
+ExecTransactionOnBehalf:testRevertSuperSafeExecOnBehalf() (gas: 3030963)
+ExecTransactionOnBehalf:testRevertZeroAddressProvidedExecTransactionOnBehalfScenarioTwo() (gas: 1739004)
+ExecTransactionOnBehalf:testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_INVALID_SIGNATURES() (gas: 2442200)
+ExecTransactionOnBehalf:testRevert_ExecTransactionOnBehalf_as_EOA_is_NOT_ROLE_with_WRONG_SIGNATURES() (gas: 2425959)
 Hierarchies:testAddSquad() (gas: 1700047)
 Hierarchies:testAddSubSquad() (gas: 2317563)
 Hierarchies:testCreateSquadThreeTiersTree() (gas: 2302579)
@@ -68,21 +68,21 @@ Hierarchies:testRevertifUpdateLimitAndExceedMaxDepthTreeLimitAndUpdateSuper() (g
 Hierarchies:testRevertifUpdateSuperToAnotherOrg() (gas: 7308560)
 Hierarchies:testTreeOrgsTreeMember() (gas: 3769349)
 Hierarchies:testUpdateSuper() (gas: 3648202)
-ModifySafeOwners:testCan_AddOwnerWithThreshold_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2432925)
-ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_EOA_is_TARGETS_LEAD() (gas: 1838127)
-ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_SAFE_is_TARGETS_LEAD() (gas: 3251561)
-ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 3178507)
-ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 3251839)
-ModifySafeOwners:testCan_AddOwnerWithThreshold_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 2443837)
-ModifySafeOwners:testCan_RemoveOwner_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2389189)
-ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_EOA_is_TARGETS_LEAD() (gas: 1805250)
-ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_SAFE_is_TARGETS_LEAD() (gas: 3211736)
-ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 1809619)
-ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 2540579)
-ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD_DifferentTree() (gas: 3193906)
-ModifySafeOwners:testCan_RemoveOwner_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 2400019)
-ModifySafeOwners:testRevertInvalidThresholdAddOwnerWithThreshold() (gas: 1878239)
-ModifySafeOwners:testRevertInvalidThresholdRemoveOwner() (gas: 1849258)
+ModifySafeOwners:testCan_AddOwnerWithThreshold_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2435295)
+ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_EOA_is_TARGETS_LEAD() (gas: 1840497)
+ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_SAFE_is_TARGETS_LEAD() (gas: 3253931)
+ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 3180877)
+ModifySafeOwners:testCan_AddOwnerWithThreshold_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 3254209)
+ModifySafeOwners:testCan_AddOwnerWithThreshold_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 2446207)
+ModifySafeOwners:testCan_RemoveOwner_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 2391559)
+ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_EOA_is_TARGETS_LEAD() (gas: 1807620)
+ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_MODIFY_OWNERS_ONLY_as_SAFE_is_TARGETS_LEAD() (gas: 3214106)
+ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_EOA_is_TARGETS_LEAD() (gas: 1811989)
+ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD() (gas: 2542949)
+ModifySafeOwners:testCan_RemoveOwner_SAFE_LEAD_as_SAFE_is_TARGETS_LEAD_DifferentTree() (gas: 3196276)
+ModifySafeOwners:testCan_RemoveOwner_SUPER_SAFE_as_SAFE_is_TARGETS_SUPER_SAFE() (gas: 2402389)
+ModifySafeOwners:testRevertInvalidThresholdAddOwnerWithThreshold() (gas: 1888343)
+ModifySafeOwners:testRevertInvalidThresholdRemoveOwner() (gas: 1859362)
 ModifySafeOwners:testRevertOwnerAlreadyExistsAddOwnerWithThreshold() (gas: 1794102)
 ModifySafeOwners:testRevertOwnerNotFoundRemoveOwner() (gas: 496050)
 ModifySafeOwners:testRevertRootSafeToAttemptTo_AddOwnerWithThreshold_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_SAFE() (gas: 3041134)
@@ -98,14 +98,14 @@ ModifySafeOwners:testRevertSafeNotRegisteredRemoveOwner_SAFE_Caller() (gas: 5086
 ModifySafeOwners:testRevertZeroAddressAddOwnerWithThreshold() (gas: 1672138)
 ModifySafeOwners:testRevertZeroAddressProvidedRemoveOwner() (gas: 3078233)
 PalmeraGuardTest:testCanPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE() (gas: 2501223)
-PalmeraGuardTest:testCanRemoveWholeTree() (gas: 4034278)
-PalmeraGuardTest:testCanRemoveWholeTreeFourthLevel() (gas: 3100718)
+PalmeraGuardTest:testCanRemoveWholeTree() (gas: 4040718)
+PalmeraGuardTest:testCanRemoveWholeTreeFourthLevel() (gas: 3127678)
 PalmeraGuardTest:testCannotDisablePalmeraGuardAfterRemoveSquad() (gas: 1887725)
 PalmeraGuardTest:testCannotDisablePalmeraGuardIfGuardEnabled() (gas: 1791316)
 PalmeraGuardTest:testCannotDisablePalmeraModuleAfterRemoveSquad() (gas: 1825557)
 PalmeraGuardTest:testCannotDisablePalmeraModuleIfGuardEnabled() (gas: 1781314)
 PalmeraGuardTest:testCannotDisconnectSafe_As_ROOTSAFE_TARGET_ITSELF_If_Have_children() (gas: 1741325)
-PalmeraGuardTest:testCannotDisconnectSafe_As_ROOTSAFE_TARGET_ROOTSAFE_SAME_TREE() (gas: 2327438)
+PalmeraGuardTest:testCannotDisconnectSafe_As_ROOTSAFE_TARGET_ROOTSAFE_SAME_TREE() (gas: 2330178)
 PalmeraGuardTest:testCannotDisconnectSafe_As_RootSafe_As_DifferentTree() (gas: 4676420)
 PalmeraGuardTest:testCannotDisconnectSafe_As_SafeLead_As_EOA() (gas: 2381349)
 PalmeraGuardTest:testCannotDisconnectSafe_As_SafeLead_As_SAFE() (gas: 2784278)
@@ -114,16 +114,16 @@ PalmeraGuardTest:testCannotDisconnectSafe_As_SuperSafe_As_SameTree() (gas: 24223
 PalmeraGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SQUAD_SAFE() (gas: 2431374)
 PalmeraGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE_ANOTHER_ORG() (gas: 4643279)
 PalmeraGuardTest:testCannotPromoteToRoot_As_ROOTSAFE_TARGET_SUPER_SAFE_ANOTHER_TREE() (gas: 4557502)
-PalmeraGuardTest:testCannotReplayAttackDisconnectedSafe() (gas: 1706175)
+PalmeraGuardTest:testCannotReplayAttackDisconnectedSafe() (gas: 1708915)
 PalmeraGuardTest:testCannotReplayAttackRemoveSquad() (gas: 1783334)
-PalmeraGuardTest:testCannotReplayAttackRemoveWholeTree() (gas: 3094002)
+PalmeraGuardTest:testCannotReplayAttackRemoveWholeTree() (gas: 3120962)
 PalmeraGuardTest:testDisablePalmeraGuard() (gas: 133458)
 PalmeraGuardTest:testDisablePalmeraModule() (gas: 104677)
-PalmeraGuardTest:testDisconnectSafeBeforeToRemoveSquad_One_Level() (gas: 2298215)
-PalmeraGuardTest:testDisconnectSafeBeforeToRemoveSquad_Two_Level() (gas: 2926482)
-PalmeraGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_ITSELF_If_Not_Have_children() (gas: 1901763)
-PalmeraGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_ROOT_SAFE() (gas: 2390060)
-PalmeraGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_SUPERSAFE_SAME_TREE() (gas: 1702484)
+PalmeraGuardTest:testDisconnectSafeBeforeToRemoveSquad_One_Level() (gas: 2300955)
+PalmeraGuardTest:testDisconnectSafeBeforeToRemoveSquad_Two_Level() (gas: 2929222)
+PalmeraGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_ITSELF_If_Not_Have_children() (gas: 1904503)
+PalmeraGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_ROOT_SAFE() (gas: 2392800)
+PalmeraGuardTest:testDisconnectSafe_As_ROOTSAFE_TARGET_SUPERSAFE_SAME_TREE() (gas: 1705224)
 PalmeraRoleDeployTest:testSetUserRoles() (gas: 32278)
 PalmeraRoleDeployTest:testSetupRolesCapabilities() (gas: 157803)
 PalmeraRolesTest:testCan_PalmeraModule_Setup_RoleContract() (gas: 18156)
@@ -135,7 +135,7 @@ PalmeraRolesTest:testCannot_ROOT_SAFE_SetRole_ROOT_SAFE_to_EOA_DifferentTree_Saf
 PalmeraRolesTest:testCannot_ROOT_SAFE_SetRole_SUPER_SAFE_to_EAO() (gas: 1679618)
 PalmeraRolesTest:testCannot_ROOT_SAFE_SetRole_SUPER_SAFE_to_SAFE() (gas: 2302439)
 PalmeraRolesTest:testCannot_SUPER_SAFE_SetRole_SAFE_LEAD_to_EAO() (gas: 1679070)
-TestDeploy:testDeploy() (gas: 6285229)
+TestDeploy:testDeploy() (gas: 6279818)
 TestDeploySafe:testTransferFundsSafe() (gas: 109166)
 TestEnableGuard:testEnablePalmeraGuard() (gas: 120802)
 TestEnableGuard:testEnablePalmeraModule() (gas: 122018)
@@ -152,8 +152,8 @@ TestPalmeraSafe:testCannot_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_Differe
 TestPalmeraSafe:testCannot_RemoveSquad_ROOT_SAFE_as_SAFE_is_TARGETS_ROOT_DifferentTree() (gas: 3093811)
 TestPalmeraSafe:testCannot_RemoveSquad_SUPER_SAFE_as_SAFE_is_not_TARGET_SUPER_SAFE_DifferentTree() (gas: 4611834)
 TestPalmeraSafe:testCannot_RemoveSquad_SUPER_SAFE_as_SAFE_is_not_TARGET_SUPER_SAFE_SameTree() (gas: 2949327)
-TestPalmeraSafe:testDisableDenyHelperList() (gas: 2666860)
+TestPalmeraSafe:testDisableDenyHelperList() (gas: 2666825)
 TestPalmeraSafe:testRemoveSquadAndCheckDisables() (gas: 1786124)
 TestPalmeraSafe:testRevertAuthForRegisterOrgTx() (gas: 14097)
-TestPalmeraSafe:testRevertSuperSafeExecOnBehalfIsDenyList() (gas: 2526610)
-TestPalmeraSafe:testRevertSuperSafeExecOnBehalfIsNotAllowList() (gas: 2435606)
+TestPalmeraSafe:testRevertSuperSafeExecOnBehalfIsDenyList() (gas: 2526575)
+TestPalmeraSafe:testRevertSuperSafeExecOnBehalfIsNotAllowList() (gas: 2435571)

src/Helpers.sol

@@ -15,10 +15,11 @@ import {
     Events
 } from "./DenyHelper.sol";
 import {SignatureDecoder} from "@safe-contracts/common/SignatureDecoder.sol";
+import {ReentrancyGuard} from "@openzeppelin/security/ReentrancyGuard.sol";
 
 /// @title Helpers
 /// @custom:security-contact [email protected]
-abstract contract Helpers is DenyHelper, SignatureDecoder {
+abstract contract Helpers is DenyHelper, SignatureDecoder, ReentrancyGuard {
     using GnosisSafeMath for uint256;
     using Address for address;
 
@@ -198,6 +199,7 @@ abstract contract Helpers is DenyHelper, SignatureDecoder {
     /// @param data Data to execute Tx
     function _executeModuleTransaction(address safe, bytes memory data)
         internal
+        nonReentrant
     {
         ISafe targetSafe = ISafe(safe);
         bool result = targetSafe.execTransactionFromModule(

src/PalmeraModule.sol

@@ -16,11 +16,10 @@ import {
     ISafe,
     ISafeProxy
 } from "./Helpers.sol";
-import {ReentrancyGuard} from "@openzeppelin/security/ReentrancyGuard.sol";
 
 /// @title Palmera Module
 /// @custom:security-contact [email protected]
-contract PalmeraModule is Auth, ReentrancyGuard, Helpers {
+contract PalmeraModule is Auth, Helpers {
     using GnosisSafeMath for uint256;
     using Address for address;
 

wake.toml

@@ -0,0 +1,11 @@
+
+[compiler.solc]
+remappings = [
+  "forge-std/=lib/forge-std/src/",
+  "ds-test/=lib/forge-std/lib/ds-test/src/",
+  "@safe-contracts=lib/safe-contracts/contracts",
+  "@openzeppelin=lib/openzeppelin-contracts/contracts",
+  "@solmate=lib/solmate/src",
+  "solenv=lib/solenv/src",
+  "@create3=lib/create3-factory/src/"
+]