Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Fix unauthorized access #89

Closed
voltrevo opened this issue Sep 18, 2023 · 1 comment · Fixed by #118
Closed

Fix unauthorized access #89

voltrevo opened this issue Sep 18, 2023 · 1 comment · Fixed by #118
Assignees

Comments

@voltrevo
Copy link
Contributor

The plugins currently allow execTransaction to be called from any address, not just the 4337 entry point.

(Perhaps this should be fixed by another plugin or some other mechanism?)

This is demonstrated by the (currently) failing test added in #88:

  itif("should not allow execTransaction from unrelated address", async () => {
    const { accountAddress, userWallet, provider } = await setupDeployedAccount(
      ethers.ZeroAddress,
      0,
      "0x",
    );

    const unrelatedWallet = ethers.Wallet.createRandom(provider);

    await receiptOf(
      userWallet.sendTransaction({
        to: unrelatedWallet.address,
        value: 100n * oneEther,
      }),
    );

    const account = SafeECDSAPlugin__factory.connect(
      accountAddress,
      unrelatedWallet, // <-------------------------------------- simply connect with unrelated wallet
    );

    const recipient = ethers.Wallet.createRandom(provider);

    await expect(
      receiptOf(account.execTransaction(recipient.address, oneEther, "0x")),
      //                ~~~~~~~~~~~~~~~ <------------------------ and call execTransaction
    ).to.eventually.rejected; // Should be rejected, but it isn't

    await expect(provider.getBalance(recipient)).to.eventually.equal(0n);
  });
@voltrevo
Copy link
Contributor Author

voltrevo commented Oct 6, 2023

I ran into another stumbling block with my compression plugin (for #30). When calling a plugin method msg.sender is the safe, rather than the caller. That's a big problem for checking that the call comes from a trusted source (ie entryPoint or self). As far as I can tell, this would require changes on the safe side.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

1 participant