Skip to content

Commit

Permalink
Use Custom Errors Instead of Revert Strings (#255)
Browse files Browse the repository at this point in the history 
This PR changes the module to use custom errors instead of revert
strings.

Previously, the entry point contract v0.6 had special support for revert
strings for improving debuggability of signature validation errors.
However, since v0.7, this is no longer the case, so we should use custom
errors in order to allow better error context and smaller bytecode.

This PR is adapted from #247, and modified to target the
`entrypoint-v0.7` branch.
  • Loading branch information
@nlordell
nlordell committed Feb 9, 2024
1 parent 414c67a commit 19ea9d6
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 19 deletions.
56 changes: 48 additions & 8 deletions modules/4337/contracts/Safe4337Module.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,57 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
address entryPoint;
}

/**
* @notice An error indicating that the entry point used when deploying a new module instance is invalid.
*/
error InvalidEntryPoint();

/**
* @notice An error indicating that the caller does not match the Safe in the corresponding user operation.
* @dev This indicates that the module is being used to validate a user operation for a Safe that did not directly
* call this module.
*/
error InvalidCaller();

/**
* @notice An error indicating that the call validating or executing a user operation was not called by the
* supported entry point contract.
*/
error UnsupportedEntryPoint();

/**
* @notice An error indicating that the user operation `callData` does not correspond to one of the two supported
* execution functions: `executeUserOp` or `executeUserOpWithErrorString`.
*/
error UnsupportedExecutionFunction(bytes4 selector);

/**
* @notice An error indicating that the user operation failed to execute successfully.
* @dev The contract reverts with this error when `executeUserOp` is used instead of bubbling up the original revert
* data. When bubbling up revert data is desirable, `executeUserOpWithErrorString` should be used instead.
*/
error ExecutionFailed();

/**
* @notice The address of the EntryPoint contract supported by this module.
*/
address public immutable SUPPORTED_ENTRYPOINT;

constructor(address entryPoint) {
require(entryPoint != address(0), "Invalid entry point");
if (entryPoint == address(0)) {
revert InvalidEntryPoint();
}

SUPPORTED_ENTRYPOINT = entryPoint;
}

/**
* @notice Validates the call is initiated by the entry point.
*/
modifier onlySupportedEntryPoint() {
require(_msgSender() == SUPPORTED_ENTRYPOINT, "Unsupported entry point");
if (_msgSender() != SUPPORTED_ENTRYPOINT) {
revert UnsupportedEntryPoint();
}
_;
}

Expand All @@ -100,14 +136,16 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
// The entry point address is appended to the calldata by the Safe in the `FallbackManager` contract,
// following ERC-2771. Because of this, the relayer may manipulate the entry point address, therefore
// we have to verify that the sender is the Safe specified in the userOperation.
require(safeAddress == msg.sender, "Invalid caller");
if (safeAddress != msg.sender) {
revert InvalidCaller();
}

// We check the execution function signature to make sure the entry point can't call any other function
// and make sure the execution of the user operation is handled by the module
require(
this.executeUserOp.selector == bytes4(userOp.callData) || this.executeUserOpWithErrorString.selector == bytes4(userOp.callData),
"Unsupported execution function id"
);
bytes4 selector = bytes4(userOp.callData);
if (selector != this.executeUserOp.selector && selector != this.executeUserOpWithErrorString.selector) {
revert UnsupportedExecutionFunction(selector);
}

// The userOp nonce is validated in the entry point (for 0.6.0+), therefore we will not check it again
validationData = _validateSignatures(userOp);
Expand All @@ -129,7 +167,9 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle
* @param operation Operation type of the user operation.
*/
function executeUserOp(address to, uint256 value, bytes memory data, uint8 operation) external onlySupportedEntryPoint {
require(ISafe(msg.sender).execTransactionFromModule(to, value, data, operation), "Execution failed");
if (!ISafe(msg.sender).execTransactionFromModule(to, value, data, operation)) {
revert ExecutionFailed();
}
}

/**
Expand Down
29 changes: 18 additions & 11 deletions modules/4337/test/erc4337/Safe4337Module.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ describe('Safe4337Module', () => {
describe('constructor', () => {
it('should revert when entry point is not specified', async () => {
const factory = await ethers.getContractFactory('Safe4337Module')
await expect(factory.deploy(ethers.ZeroAddress)).to.be.revertedWith('Invalid entry point')
await expect(factory.deploy(ethers.ZeroAddress)).to.be.revertedWithCustomError(factory, 'InvalidEntryPoint')
})
})

Expand Down Expand Up @@ -166,7 +166,10 @@ describe('Safe4337Module', () => {
expect(await safeFromEntryPoint.validateUserOp.staticCall(userOp, ethers.ZeroHash, 0)).to.eq(0)

const otherSafe = (await makeSafeModule(user)).connect(entryPointImpersonator)
await expect(otherSafe.validateUserOp.staticCall(userOp, ethers.ZeroHash, 0)).to.be.revertedWith('Invalid caller')
await expect(otherSafe.validateUserOp.staticCall(userOp, ethers.ZeroHash, 0)).to.be.revertedWithCustomError(
safeModule,
'InvalidCaller',
)
})

it('should revert when calling an unsupported Safe method', async () => {
Expand All @@ -189,9 +192,9 @@ describe('Safe4337Module', () => {

const entryPointAddress = await ethers.getSigner(await entryPoint.getAddress())
const safeFromEntryPoint = safeModule.connect(entryPointAddress)
await expect(safeFromEntryPoint.validateUserOp.staticCall(userOp, ethers.ZeroHash, 0)).to.be.revertedWith(
'Unsupported execution function id',
)
await expect(safeFromEntryPoint.validateUserOp.staticCall(userOp, ethers.ZeroHash, 0))
.to.be.revertedWithCustomError(safeModule, 'UnsupportedExecutionFunction')
.withArgs(ethers.dataSlice(callData, 0, 4))
})

it('should revert when not called from the trusted entrypoint', async () => {
Expand All @@ -204,7 +207,10 @@ describe('Safe4337Module', () => {
signature,
})

await expect(safeModule.connect(untrusted).validateUserOp(userOp, ethers.ZeroHash, 0)).to.be.revertedWith('Unsupported entry point')
await expect(safeModule.connect(untrusted).validateUserOp(userOp, ethers.ZeroHash, 0)).to.be.revertedWithCustomError(
safeModule,
'UnsupportedEntryPoint',
)
})

it('should return correct validAfter and validUntil timestamps', async () => {
Expand Down Expand Up @@ -245,18 +251,19 @@ describe('Safe4337Module', () => {
describe('execUserOp', () => {
it('should revert when not called from the trusted entrypoint', async () => {
const { untrusted, safeModule } = await setupTests()
await expect(safeModule.connect(untrusted).executeUserOp(ethers.ZeroAddress, 0, '0x', 0)).to.be.revertedWith(
'Unsupported entry point',
await expect(safeModule.connect(untrusted).executeUserOp(ethers.ZeroAddress, 0, '0x', 0)).to.be.revertedWithCustomError(
safeModule,
'UnsupportedEntryPoint',
)
})
})

describe('execUserOpWithErrorString', () => {
it('should revert when not called from the trusted entrypoint', async () => {
const { untrusted, safeModule } = await setupTests()
await expect(safeModule.connect(untrusted).executeUserOpWithErrorString(ethers.ZeroAddress, 0, '0x', 0)).to.be.revertedWith(
'Unsupported entry point',
)
await expect(
safeModule.connect(untrusted).executeUserOpWithErrorString(ethers.ZeroAddress, 0, '0x', 0),
).to.be.revertedWithCustomError(safeModule, 'UnsupportedEntryPoint')
})
})
})

0 comments on commit 19ea9d6

Please sign in to comment.