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

Fix unauthorized access #118

Merged
merged 4 commits into from
Oct 12, 2023
Merged

Fix unauthorized access #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5161a78
Fix unauthorized access
voltrevo Oct 10, 2023
f4a56f1
Extract setupDeployedAccount
voltrevo Oct 11, 2023
31255dd
Test that unrelated address can't call execTransaction
voltrevo Oct 11, 2023
454f7fc
Remove unused imports
voltrevo Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions account-integrations/safe/src/SafeBlsPlugin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
pragma solidity >=0.7.0 <0.9.0;
pragma abicoder v2;

import {HandlerContext} from "safe-contracts/contracts/handler/HandlerContext.sol";

import {BaseAccount} from "account-abstraction/contracts/core/BaseAccount.sol";
import {IEntryPoint, UserOperation} from "account-abstraction/contracts/interfaces/IEntryPoint.sol";
import {BLS} from "account-abstraction/contracts/samples/bls/lib/hubble-contracts/contracts/libs/BLS.sol";
Expand All @@ -19,7 +21,7 @@ interface ISafe {

error IncorrectSignatureLength(uint256 length);

contract SafeBlsPlugin is BaseAccount {
contract SafeBlsPlugin is BaseAccount, HandlerContext {
// TODO: Use EIP 712 for domain separation
bytes32 public constant BLS_DOMAIN = keccak256("eip4337.bls.domain");
address public immutable myAddress;
Expand Down Expand Up @@ -50,7 +52,7 @@ contract SafeBlsPlugin is BaseAccount {
address to,
uint256 value,
bytes calldata data
) external payable {
) external payable fromThisOrEntryPoint {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does execTransaction need to allow this? (The safe can call its function execTransactionFromModule directly if needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think I was just a bit hazy on that when I originally wrote this code. Will update.

address payable safeAddress = payable(msg.sender);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is msg.sender always going to be the safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm not sure. I think yes if the function is always executed via safe's module system. The msg.sender here is not part of the changes in this PR, but it looks wrong to me too.

I think the original source of using msg.sender here is safe's 4337 example. I opened a PR over there to change it to use this, curious to see what they say: safe-global/safe-smart-account#682.

ISafe safe = ISafe(safeAddress);
require(
Expand Down Expand Up @@ -124,4 +126,12 @@ contract SafeBlsPlugin is BaseAccount {
);
}
}

modifier fromThisOrEntryPoint() {
require(
_msgSender() == address(this) ||
_msgSender() == _entryPoint
);
_;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the virtual function _requireFromEntryPoint(...) in BaseAccount be overridden (to use _msgSender())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah probably. I'd suggest we make something like Safe4337Base we can inherit from, which uses BaseAccount and HandlerContext and implement this override.

I'm wary of inheritance in general though. I've always avoided it. However, it seems popular in the smart contracts we're working with, so I'm inclined to lean into it for now. Wdyt?

}
14 changes: 12 additions & 2 deletions account-integrations/safe/src/SafeECDSAPlugin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
pragma solidity >=0.7.0 <0.9.0;
pragma abicoder v2;

import {HandlerContext} from "safe-contracts/contracts/handler/HandlerContext.sol";

import {BaseAccount} from "account-abstraction/contracts/core/BaseAccount.sol";
import {IEntryPoint, UserOperation} from "account-abstraction/contracts/interfaces/IEntryPoint.sol";
import {UserOperation} from "account-abstraction/contracts/interfaces/IEntryPoint.sol";
Expand All @@ -23,7 +25,7 @@ struct ECDSAOwnerStorage {
address owner;
}

contract SafeECDSAPlugin is BaseAccount {
contract SafeECDSAPlugin is BaseAccount, HandlerContext {
using ECDSA for bytes32;

mapping(address => ECDSAOwnerStorage) public ecdsaOwnerStorage;
Expand Down Expand Up @@ -55,7 +57,7 @@ contract SafeECDSAPlugin is BaseAccount {
address to,
uint256 value,
bytes calldata data
) external payable {
) external payable fromThisOrEntryPoint {
address payable safeAddress = payable(msg.sender);
ISafe safe = ISafe(safeAddress);
require(
Expand Down Expand Up @@ -123,4 +125,12 @@ contract SafeECDSAPlugin is BaseAccount {
);
}
}

modifier fromThisOrEntryPoint() {
require(
_msgSender() == address(this) ||
_msgSender() == _entryPoint
);
_;
}
}
14 changes: 12 additions & 2 deletions account-integrations/safe/src/SafeWebAuthnPlugin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.8.0 <0.9.0;

import {HandlerContext} from "safe-contracts/contracts/handler/HandlerContext.sol";

import {BaseAccount} from "account-abstraction/contracts/core/BaseAccount.sol";
import {IEntryPoint, UserOperation} from "account-abstraction/contracts/interfaces/IEntryPoint.sol";
import {WebAuthn} from "wax/primitives/src/WebAuthn.sol";
Expand All @@ -16,7 +18,7 @@ interface ISafe {
) external returns (bool success);
}

contract SafeWebAuthnPlugin is BaseAccount, WebAuthn {
contract SafeWebAuthnPlugin is BaseAccount, WebAuthn, HandlerContext {
address public immutable myAddress;
address private immutable _entryPoint;
uint256[2] private _publicKey;
Expand Down Expand Up @@ -45,7 +47,7 @@ contract SafeWebAuthnPlugin is BaseAccount, WebAuthn {
address to,
uint256 value,
bytes calldata data
) external payable {
) external payable fromThisOrEntryPoint {
address payable safeAddress = payable(msg.sender);
ISafe safe = ISafe(safeAddress);
require(
Expand Down Expand Up @@ -189,4 +191,12 @@ contract SafeWebAuthnPlugin is BaseAccount, WebAuthn {
);
}
}

modifier fromThisOrEntryPoint() {
require(
_msgSender() == address(this) ||
_msgSender() == _entryPoint
);
_;
}
}
82 changes: 64 additions & 18 deletions account-integrations/safe/test/hardhat/integration/SafeECDSAPlugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import hre from "hardhat";
import { expect } from "chai";
import { AddressZero } from "@ethersproject/constants";
import { getBytes, concat, resolveProperties, ethers } from "ethers";
import { getBytes, resolveProperties, ethers } from "ethers";
import { UserOperationStruct } from "@account-abstraction/contracts";
import { getUserOpHash } from "@account-abstraction/utils";
import { calculateProxyAddress } from "../utils/calculateProxyAddress";
import {
SafeECDSAFactory__factory,
SafeECDSAPlugin__factory,
Expand All @@ -26,6 +23,8 @@ const BUNDLER_URL = process.env.ERC4337_TEST_BUNDLER_URL;
const NODE_URL = process.env.ERC4337_TEST_NODE_URL;
const MNEMONIC = process.env.MNEMONIC;

const oneEther = ethers.parseEther("1");

describe("SafeECDSAPlugin", () => {
const setupTests = async () => {
const bundlerProvider = new ethers.JsonRpcProvider(BUNDLER_URL);
Expand Down Expand Up @@ -54,14 +53,11 @@ describe("SafeECDSAPlugin", () => {
};
};

/**
* This test verifies a ERC4337 transaction succeeds when sent via a plugin
* The user operation deploys a Safe with the ERC4337 plugin and a handler
* and executes a transaction, thus verifying two things:
* 1. Deployment of the Safe with the ERC4337 plugin and handler is possible
* 2. Executing a transaction is possible
*/
itif("should pass the ERC4337 validation", async () => {
async function setupDeployedAccount(
to: ethers.AddressLike,
value: ethers.BigNumberish,
data: ethers.BytesLike,
) {
const { singleton, provider, bundlerProvider, userWallet, entryPoints } =
await setupTests();
const ENTRYPOINT_ADDRESS = entryPoints[0];
Expand Down Expand Up @@ -106,7 +102,7 @@ describe("SafeECDSAPlugin", () => {
const userOpCallData =
SafeECDSAPlugin__factory.createInterface().encodeFunctionData(
"execTransaction",
[recipient.address, transferAmount, "0x00"],
[to, value, data],
);

// Native tokens for the pre-fund 💸
Expand Down Expand Up @@ -160,13 +156,63 @@ describe("SafeECDSAPlugin", () => {
// `;
// console.log(DEBUG_MESSAGE);

const recipientBalanceBefore = await provider.getBalance(recipient.address);

await sendUserOpAndWait(userOperation, ENTRYPOINT_ADDRESS, bundlerProvider);

const recipientBalanceAfter = await provider.getBalance(recipient.address);
return {
provider,
bundlerProvider,
entryPoint: ENTRYPOINT_ADDRESS,
userWallet,
accountAddress,
};
}

/**
* This test verifies a ERC4337 transaction succeeds when sent via a plugin
* The user operation deploys a Safe with the ERC4337 plugin and a handler
* and executes a transaction, thus verifying two things:
* 1. Deployment of the Safe with the ERC4337 plugin and handler is possible
* 2. Executing a transaction is possible
*/
itif("should pass the ERC4337 validation", async () => {
const recipient = ethers.Wallet.createRandom();

const { provider } = await setupDeployedAccount(
recipient.address,
oneEther,
"0x",
);

expect(await provider.getBalance(recipient.address)).to.equal(oneEther);
});

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,
);

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

await expect(
receiptOf(account.execTransaction(recipient.address, oneEther, "0x")),
).to.eventually.rejected;

const expectedRecipientBalance = recipientBalanceBefore + transferAmount;
expect(recipientBalanceAfter).to.equal(expectedRecipientBalance);
await expect(provider.getBalance(recipient)).to.eventually.equal(0n);
});
});