diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index 7698fef93..855309572 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -69,13 +69,47 @@ 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; } @@ -83,7 +117,9 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle * @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(); + } _; } @@ -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); @@ -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(); + } } /** diff --git a/modules/4337/test/erc4337/Safe4337Module.spec.ts b/modules/4337/test/erc4337/Safe4337Module.spec.ts index ba69fe46f..38ed19eaa 100644 --- a/modules/4337/test/erc4337/Safe4337Module.spec.ts +++ b/modules/4337/test/erc4337/Safe4337Module.spec.ts @@ -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') }) }) @@ -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 () => { @@ -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 () => { @@ -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 () => { @@ -245,8 +251,9 @@ 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', ) }) }) @@ -254,9 +261,9 @@ describe('Safe4337Module', () => { 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') }) }) })