An attacker can steal reverted transaction signatures and use them to his advantage

[hats-bug-reporter[bot]]

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

Description:

Description

The function execTransactionOnBehalf of the contract PalmeraModule:

• Can be execute by anyone with the rights signatures
• The signatures don't have expiry
• If the transaction reverts the signatures are exposed and the nonce don't increment, then they are still valid

These arguments make it possible that the signatures of the revert calls to execTransactionOnBehalf remain valid and public and an attacker could use these signatures to send the same transaction generating some benefit for the attacker

Attack Scenario

• A EOA caller call execTransactionOnBehalf with the rights owners signatures to make a swap in uniswap(buy 1 ETH for 3400 USD)
• This call reverts by market conditions(The price of ETH goes up, 4000 USD)
• Later, price of ETH goes down, 2500 USD
• An attacker have the valid signatures to call execTransactionOnBehalf with the price of 1 ETH for 3400 USD, and call it
• Finally the contract executes a purchase of one ether at 3400 USD while the market price was 1000 USD

Recommendation

As the behavior of `execTransactionFromModule of ModuleManager.sol of Safe if the call is not success, it should not revert:

@@ -183,7 +183,6 @@ contract PalmeraModule is Auth, Helpers {
         result =
             safeTarget.execTransactionFromModule(to, value, data, operation);

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

Attachments

PoC

PalmeraModule.sol#L183-L186:

        result =
            safeTarget.execTransactionFromModule(to, value, data, operation);

        if (!result) revert Errors.TxOnBehalfExecutedFailed();

[alfredolopez80]

Ok, first point @rotcivegaf this is not a issue because is a expected behavior, that's why is because we work with the safe signing flow, if we not permit a any EOA, can call a executionOnBehalf with the rights signatures (as you mention), the user of the RootSafe/SuperSafe, need to signing twice the transaction, and this is very bad UI/UX experience, so we decide to follow the Safe Approach and permit, if the EOA have the right signature right rigth arguments, can execute and pay gas fee of the executuon On Behalf!!!

About the period of valid signature, we recomend our user handle this with ERC4337 control the valid signature with timestamp (Valid After/ Valid until)) with module ERC-4337 of Safe, we think is the better approach at the moment.

https://github.com/safe-global/safe-modules/blob/9a18245f546bf2a8ed9bdc2b04aae44f949ec7a0/modules/4337/contracts/Safe4337Module.sol#L45