Safe-based accounts are not 4337-compliant

[derekchiang]

This is not an issue with 4337 per se but since there's a reference implementation for a Safe-based account, I thought I'd bring up this issue that I discovered with @hazim-j.

Basically, the account uses a fallback handler to implement validateUserOp, which is the only way if the account is to be a Safe.

However, Safe's fallback() uses the GAS opcode: https://github.com/safe-global/safe-contracts/blob/92de184083c6c783ca5518816ee503e67261dd3a/contracts/base/FallbackManager.sol#L45

The spec says that (validateUserOp) Must not use GAS opcode (unless followed immediately by one of { CALL, DELEGATECALL, CALLCODE, STATICCALL }.)

The call stack for the code I linked looks like:

- gas
- opcode for add
- opcode for calldatasize
...
- call

Therefore UserOps from Safe-based accounts will be rejected by 4337-compliant bundlers, since the CALL is not immediately following the GAS.

Any thoughts on whether the rules can be somehow relaxed to accommodate cases like this, where the use of GAS is actually safe?

EDIT: Safe-based accounts are indeed not 4337-compliant but not for the reason I stated. See leekt's comment here.

[derekchiang]

This might be red herring after some more debugging with @leekt and @hazim-j. Closing for now and will re-open if the issue turns out to be real.

[hazim-j]

Not ruling this out yet and would be good if someone else is able to confirm my findings. I added some debug logs around the bundlerCollectorTracer and found that the issue was coming from using PUSH2 right after GAS:

This was also the related enter debug log:

gas=82663type=DELEGATECALLto=0xd9db270c1b5e3bd161e8c8503c55ceabee709552in=0x5229073f000000000000000000000000ac438821d2e1d1d574decd1a30a1e2fdcc6bd2a700000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000004380825d1fc00000000000000000000000000000000000000000000000000000000000000801635d0bcee9196d3110a3c7e2c08730968183367ed9418dabdec011265a432ac0000000000000000000000000000000000"

The address 0xd9db270c1b5e3bd161e8c8503c55ceabee709552 is the gnosis safe implementation. The function selector 0x5229073f seems to be a call to execTransactionFromModuleReturnData. And somewhere within that call stack is the forbidden use of GAS + PUSH2.

[leekt]

Gnosis safe contract is not erc4337 compatible because of this part here https://github.com/safe-global/safe-contracts/blob/v1.3.0/contracts/base/ModuleManager.sol#L70 it passes gasleft() to execute() function which means it will mess up the opcode orders

this issue does not exist with latest commit of safe contracts but i think it's not deployed yet https://github.com/safe-global/safe-contracts/blob/92de184083c6c783ca5518816ee503e67261dd3a/contracts/base/ModuleManager.sol#L72

meaning that we need to re-open this issue

[derekchiang]

Reopening and cc-ing @yoavw and @drortirosh. Is there any way the opcode checking policy can be relaxed for something like this?

[derekchiang]

So it turns out that the Safe team explicitly acknowledged this issue (and fixed it in one of their recent commits): https://github.com/safe-global/safe-contracts/issues/459

[derekchiang]

Quoting Yoav in DM:

In any case, we can't enable this. We were only able to support GAS+CALL (which initially also wasn't allowed) since we know that the gas value is consumed from the stack immediately and cannot be used by the code in any other way. Passing it as an arg to another call would violate this protection and enable many DoS vectors.

Closing for now.