executeTransaction on SafeProtocolManager fails when executing Safe's self call

[porco-rosso-j]

With SafeProtocolManager, executeTransaction that tries to carry out a self-call (safeProtocolAction.to == safeAddress) on Safe fails, for example, calling swapOwner() in OwnerManager via a registered module, due to the require at the line of 96. see below.

safe-core-protocol/contracts/SafeProtocolManager.sol

Line 96 in 0dc36e0

if (safeProtocolAction.to == address(this) || safeProtocolAction.to == safeAddress) {

What is the rationale behind this design? As I'm trying to build a recovery plugin, I wonder if there is a walkaround or it's possible to lift this off so that an external module can add, remove, and swap owners via safe-core-protocol.

Btw, executeRootAccess(), which doesn't care about what address Safe calls, can't resolve this as the delegatecall will be blocked by authorized modifier in SelfAuthorized contract in Safe.

[mmv08]

Regarding the issue in the title the rationale can be found here 5afe/safe-core-protocol-specs#7

[porco-rosso-j]

I'm not sure if I understood the discussion fully as I'm not familiar with both the code base and contexts, but so seems like rootAccess was added to allow modules to let Safe send txs to the safe itself as well as executing delegatecall to external contracts.

What do you think about adding a boolean like isDelegateCall in SafeRootAccess type struct like below.

struct SafeRootAccess {
    SafeProtocolAction action;
    uint256 nonce;
    bytes32 metadataHash;
    bool isDelegateCall; // here
}

and make the execTransactionFromModuleReturnData call in executeRootAccess() flexibly able to change the call type according to the boolean value.

      unit8 operation = safeProtocolAction.isDelegateCall == true ? 1: 0;
      
      (success, data) = safe.execTransactionFromModuleReturnData(
            safeProtocolAction.to,
            safeProtocolAction.value,
            safeProtocolAction.data,
            operation
        );

[mmv08]

I'd personally just allow passing the operation for the root access plugins. Nevertheless, we'll discuss that with the team.

[porco-rosso-j]

Isn't that what my code does, no? Currently, you can't specify the call type

safe-core-protocol/contracts/SafeProtocolManager.sol

Line 153 in 0dc36e0

1

Anyway, would be nice if this will be resolved soon!

[mmv08]

Isn't that what my code does, no? Currently, you can't specify the call type

safe-core-protocol/contracts/SafeProtocolManager.sol

Line 153 in 0dc36e0

1

Anyway, would be nice if this will be resolved soon!

That's why I said "I'd just allow" sir