[Safe 1.5.0] `execTransactionFromModuleReturnData` Will Return Guard `returndata`

[nlordell]

Description

EVM returndata* opcodes only operate on the return data of the last call. execTransactionFromModuleReturnData seems to assume that the last call to executeTransactionFromModule will be the transaction specified by the module, which is not true in case there is a guard. In that case, execTransactionFromModuleReturnData will return the returndata from the Guard.checkAfterExecution instead of the actual module transaction.

Additional context

Sample test that demonstrates this bug:

```diff diff --git a/test/core/Safe.GuardManager.spec.ts b/test/core/Safe.GuardManager.spec.ts index c760b25..8a6aadf 100644 --- a/test/core/Safe.GuardManager.spec.ts +++ b/test/core/Safe.GuardManager.spec.ts @@ -365,5 +365,38 @@ describe("GuardManager", () => { expect(await validGuardMock.invocationCountForCalldata(checkAfterExecutionTxData)).to.equal(1); }); + + it.only("Returns expected from contract on successs", async () => { + const { + safe, + validGuardMock, + signers: [user1, user2], + } = await setupWithTemplate(); + const validGuardMockAddress = await validGuardMock.getAddress(); + + const guardInterface = (await hre.ethers.getContractAt("Guard", validGuardMockAddress)).interface; + const checkModuleTxData = guardInterface.encodeFunctionData("checkModuleTransaction", [ + user1.address, + 0, + "0xbeef73", + 1, + user1.address, + ]); + await validGuardMock.givenCalldataReturnBytes32(checkModuleTxData, ethers.ZeroHash); + const checkAfterExecutionTxData = guardInterface.encodeFunctionData("checkAfterExecution", [ethers.ZeroHash, true]); + await validGuardMock.givenCalldataReturn(checkAfterExecutionTxData, "0x1337"); + + const mock = await getMock(); + const mockAddress = await mock.getAddress(); + await mock.givenCalldataReturn("0xbeef73", "0xdeaddeed"); + + // NOTE(nlordell): Removing the guard makes the test pass... + // await executeContractCallWithSigners(safe, safe, "setGuard", [AddressZero], [user2]); + await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user2]); + await expect(await safe.execTransactionFromModuleReturnData.staticCall(mockAddress, 0, "0xbeef73", 0)).to.be.deep.eq([ + true, + "0xdeaddeed", + ]); + }); }); }); ```

[mmv08]

I also created a test here: https://github.com/safe-global/safe-contracts/tree/bug/execTransactionFromModuleReturnData-mit-guard haven't seen you added a test 🤦

[mmv08]

I also created a test here: https://github.com/safe-global/safe-contracts/tree/bug/execTransactionFromModuleReturnData-mit-guard haven't seen you added a test 🤦

I also added a PoC solution to my branch. Was a little curious to try the approach

[remedcu]

That was an elegant solution @mmv08 to assign the return data before the post-execution checks!