From 08ad285a44a2ec9e89f1a973fa93a57d233514b7 Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Thu, 14 Dec 2023 05:47:51 +0400 Subject: [PATCH 1/3] add ways to catch AA33 with revert reason --- contracts/sponsorship/SponsorshipPaymaster.sol | 4 ++-- .../biconomy-sponsorship-paymaster-specs.ts | 14 ++++++++------ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/contracts/sponsorship/SponsorshipPaymaster.sol b/contracts/sponsorship/SponsorshipPaymaster.sol index 43cd4aa..f9e3427 100644 --- a/contracts/sponsorship/SponsorshipPaymaster.sol +++ b/contracts/sponsorship/SponsorshipPaymaster.sol @@ -177,8 +177,7 @@ contract SponsorshipPaymaster is ) public override nonReentrant { if (withdrawAddress == address(0)) revert CanNotWithdrawToZeroAddress(); uint256 currentBalance = paymasterIdBalances[msg.sender]; - if (amount > currentBalance) - revert InsufficientBalance(amount, currentBalance); + require(amount <= currentBalance, "Sponsorship Paymaster: Insufficient withdrawable funds"); paymasterIdBalances[msg.sender] = currentBalance - amount; entryPoint.withdrawTo(withdrawAddress, amount); emit GasWithdrawn(msg.sender, withdrawAddress, amount); @@ -336,6 +335,7 @@ contract SponsorshipPaymaster is effectiveCost, paymasterIdBalances[paymasterId] ); + // require(effectiveCost <= paymasterIdBalances[paymasterId], "BTPM: account does not have enough token balance"); context = abi.encode( paymasterId, diff --git a/test/sponsorship-paymaster/biconomy-sponsorship-paymaster-specs.ts b/test/sponsorship-paymaster/biconomy-sponsorship-paymaster-specs.ts index fd5e331..ee4c8d8 100644 --- a/test/sponsorship-paymaster/biconomy-sponsorship-paymaster-specs.ts +++ b/test/sponsorship-paymaster/biconomy-sponsorship-paymaster-specs.ts @@ -158,9 +158,12 @@ describe("EntryPoint with VerifyingPaymaster Singleton", function () { console.log("paymaster Id ", paymasterId); const userOp = await getUserOpWithPaymasterInfo(paymasterId); - await expect( - entryPoint.callStatic.simulateValidation(userOp) - ).to.be.revertedWithCustomError(entryPoint, "FailedOp"); + // Review + // We would like it to be reverted with proper revert reason + // TODO: look into it why it goes into OOG in EP + await expect(entryPoint.callStatic.simulateValidation(userOp)) + .to.be.revertedWithCustomError(entryPoint, "FailedOp") + .withArgs(0, "AA23 reverted (or OOG)"); }); it("succeed with valid signature", async () => { @@ -387,9 +390,8 @@ describe("EntryPoint with VerifyingPaymaster Singleton", function () { sponsorshipPaymaster .connect(depositorSigner) .withdrawTo(paymasterId, parseEther("1.1")) - ).to.be.revertedWithCustomError( - sponsorshipPaymaster, - "InsufficientBalance" + ).to.be.revertedWith( + "Sponsorship Paymaster: Insufficient withdrawable funds" ); }); }); From 037c9e1fe9e9e010281653c6de04a563c08b4192 Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Tue, 26 Dec 2023 21:34:43 +0400 Subject: [PATCH 2/3] resolve conflicts + revert strings in validate paymaster userop --- .../sponsorship/SponsorshipPaymaster.sol | 11 +-- .../biconomy-sponsorship-paymaster-specs.ts | 69 ++++++++++++++++++- 2 files changed, 69 insertions(+), 11 deletions(-) diff --git a/contracts/sponsorship/SponsorshipPaymaster.sol b/contracts/sponsorship/SponsorshipPaymaster.sol index d1fec4a..d5d04b3 100644 --- a/contracts/sponsorship/SponsorshipPaymaster.sol +++ b/contracts/sponsorship/SponsorshipPaymaster.sol @@ -314,7 +314,7 @@ contract SponsorshipPaymaster is ); uint256 sigLength = signature.length; // we only "require" it here so that the revert reason on invalid signature will be of "VerifyingPaymaster", and not "ECDSA" - if (sigLength != 65) revert InvalidPaymasterSignatureLength(sigLength); + require(sigLength == 65, "Sponsorship Paymaster:invalid signature length"); //don't revert on signature failure: return SIG_VALIDATION_FAILED if ( verifyingSigner != hash.toEthSignedMessageHash().recover(signature) @@ -323,19 +323,14 @@ contract SponsorshipPaymaster is return (context, _packValidationData(true, validUntil, validAfter)); } - require(priceMarkup <= 2e6, "Verifying PM:high markup %"); + require(priceMarkup <= 2e6, "Sponsorship Paymaster: high markup %"); uint32 dynamicMarkup = MathLib.maxuint32(priceMarkup, fixedPriceMarkup); uint256 effectiveCost = (requiredPreFund * dynamicMarkup) / PRICE_DENOMINATOR; - if (effectiveCost > paymasterIdBalances[paymasterId]) - revert InsufficientBalance( - effectiveCost, - paymasterIdBalances[paymasterId] - ); - // require(effectiveCost <= paymasterIdBalances[paymasterId], "BTPM: account does not have enough token balance"); + require(effectiveCost <= paymasterIdBalances[paymasterId], "Sponsorship Paymaster: paymasterId does not have enough deposit"); context = abi.encode( paymasterId, diff --git a/test/sponsorship-paymaster/biconomy-sponsorship-paymaster-specs.ts b/test/sponsorship-paymaster/biconomy-sponsorship-paymaster-specs.ts index c12dd64..b49c15c 100644 --- a/test/sponsorship-paymaster/biconomy-sponsorship-paymaster-specs.ts +++ b/test/sponsorship-paymaster/biconomy-sponsorship-paymaster-specs.ts @@ -44,7 +44,7 @@ describe("EntryPoint with VerifyingPaymaster Singleton", function () { let ethersSigner; let offchainSigner: Signer, deployer: Signer, feeCollector: Signer; - let secondFundingId: Signer; + let secondFundingId: Signer, thirdFundingId: Signer; let sponsorshipPaymaster: SponsorshipPaymaster; let smartWalletImp: BiconomyAccountImplementation; @@ -61,6 +61,7 @@ describe("EntryPoint with VerifyingPaymaster Singleton", function () { depositorSigner = ethersSigner[2]; feeCollector = ethersSigner[3]; secondFundingId = ethersSigner[4]; + thirdFundingId = ethersSigner[5]; walletOwner = deployer; // ethersSigner[0]; const offchainSignerAddress = await offchainSigner.getAddress(); @@ -558,9 +559,71 @@ describe("EntryPoint with VerifyingPaymaster Singleton", function () { paymasterIdBalanceDiff.mul(BigNumber.from(1)).div(BigNumber.from(11)) ); }); + + it("fails for fundingId which does not have enough deposit", async () => { + const fundingId = await thirdFundingId.getAddress(); + + await sponsorshipPaymaster + .connect(deployer) + .setUnaccountedEPGasOverhead(18500); + + // do not deposit + + const signer = await sponsorshipPaymaster.verifyingSigner(); + const offchainSignerAddress = await offchainSigner.getAddress(); + expect(signer).to.be.equal(offchainSignerAddress); + + const userOp1 = await fillAndSign( + { + sender: walletAddress, + verificationGasLimit: 200000, + }, + walletOwner, + entryPoint, + "nonce" + ); + + const hash = await sponsorshipPaymaster.getHash( + userOp1, + fundingId, + MOCK_VALID_UNTIL, + MOCK_VALID_AFTER, + dynamicMarkup + ); + const sig = await offchainSigner.signMessage(arrayify(hash)); + const userOp = await fillAndSign( + { + ...userOp1, + paymasterAndData: hexConcat([ + paymasterAddress, + ethers.utils.defaultAbiCoder.encode( + ["address", "uint48", "uint48", "uint32"], + [fundingId, MOCK_VALID_UNTIL, MOCK_VALID_AFTER, dynamicMarkup] + ), + sig, + ]), + }, + walletOwner, + entryPoint, + "nonce" + ); + + const signatureWithModuleAddress = ethers.utils.defaultAbiCoder.encode( + ["bytes", "address"], + [userOp.signature, ecdsaModule.address] + ); + userOp.signature = signatureWithModuleAddress; + + await expect(entryPoint.callStatic.simulateValidation(userOp)) + .to.be.revertedWithCustomError(entryPoint, "FailedOp") + .withArgs( + 0, + "AA33 reverted: Sponsorship Paymaster: paymasterId does not have enough deposit" + ); + }); }); - describe("Verifying Paymaster - read methods and state checks", () => { + describe("Sponsorship Paymaster - read methods and state checks", () => { it("Should parse data properly", async () => { const paymasterAndData = hexConcat([ paymasterAddress, @@ -608,7 +671,7 @@ describe("EntryPoint with VerifyingPaymaster Singleton", function () { }); }); - describe("Verifying Paymaster - deposit and withdraw tests", () => { + describe("Sponsorship Paymaster - deposit and withdraw tests", () => { it("Deposits into the specified address", async () => { const paymasterId = await depositorSigner.getAddress(); From ec2563f83ceee12be66e3713484e111b1830cd6a Mon Sep 17 00:00:00 2001 From: livingrockrises <90545960+livingrockrises@users.noreply.github.com> Date: Fri, 29 Dec 2023 12:30:21 +0400 Subject: [PATCH 3/3] feat:refactor error messages --- contracts/sponsorship/SponsorshipPaymaster.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/sponsorship/SponsorshipPaymaster.sol b/contracts/sponsorship/SponsorshipPaymaster.sol index d5d04b3..4f59621 100644 --- a/contracts/sponsorship/SponsorshipPaymaster.sol +++ b/contracts/sponsorship/SponsorshipPaymaster.sol @@ -177,7 +177,7 @@ contract SponsorshipPaymaster is ) public override nonReentrant { if (withdrawAddress == address(0)) revert CanNotWithdrawToZeroAddress(); uint256 currentBalance = paymasterIdBalances[msg.sender]; - require(amount <= currentBalance, "Sponsorship Paymaster: Insufficient withdrawable funds"); + require(amount <= currentBalance, "Sponsorship Paymaster: Insufficient funds to withdraw from gas tank"); paymasterIdBalances[msg.sender] = currentBalance - amount; entryPoint.withdrawTo(withdrawAddress, amount); emit GasWithdrawn(msg.sender, withdrawAddress, amount); @@ -314,7 +314,7 @@ contract SponsorshipPaymaster is ); uint256 sigLength = signature.length; // we only "require" it here so that the revert reason on invalid signature will be of "VerifyingPaymaster", and not "ECDSA" - require(sigLength == 65, "Sponsorship Paymaster:invalid signature length"); + require(sigLength == 65, "Sponsorship Paymaster:invalid paymaster signature length"); //don't revert on signature failure: return SIG_VALIDATION_FAILED if ( verifyingSigner != hash.toEthSignedMessageHash().recover(signature)