diff --git a/contracts/common/Errors.sol b/contracts/common/Errors.sol index 998492a..58bf40f 100644 --- a/contracts/common/Errors.sol +++ b/contracts/common/Errors.sol @@ -32,6 +32,36 @@ contract BiconomySponsorshipPaymasterErrors { */ error VerifyingSignerCanNotBeContract(); + /** + * @notice Throws when ETH withdrawal fails + */ + error WithdrawalFailed(); + + /** + * @notice Throws when insufficient funds to withdraw + */ + error InsufficientFundsInGasTank(); + + /** + * @notice Throws when invalid signature length in paymasterAndData + */ + error InvalidSignatureLength(); + + /** + * @notice Throws when invalid signature length in paymasterAndData + */ + error InvalidPriceMarkup(); + + /** + * @notice Throws when insufficient funds for paymasterid + */ + error InsufficientFundsForPaymasterId(); + + /** + * @notice Throws when calling deposit() + */ + error UseDepositForInstead(); + /** * @notice Throws when trying to withdraw to address(0) */ diff --git a/contracts/sponsorship/SponsorshipPaymasterWithPremium.sol b/contracts/sponsorship/SponsorshipPaymasterWithPremium.sol index 2e3abf4..3011264 100644 --- a/contracts/sponsorship/SponsorshipPaymasterWithPremium.sol +++ b/contracts/sponsorship/SponsorshipPaymasterWithPremium.sol @@ -39,7 +39,6 @@ contract BiconomySponsorshipPaymaster is address public verifyingSigner; address public feeCollector; - uint48 public postopCost; uint32 private constant PRICE_DENOMINATOR = 1e6; // note: could rename to PAYMASTER_ID_OFFSET @@ -55,8 +54,11 @@ contract BiconomySponsorshipPaymaster is ) BasePaymaster(_owner, _entryPoint) { - // TODO - // Check for zero address + if (_verifyingSigner == address(0)) { + revert VerifyingSignerCanNotBeZero(); + } else if (_feeCollector == address(0)) { + revert FeeCollectorCanNotBeZero(); + } verifyingSigner = _verifyingSigner; feeCollector = _feeCollector; } @@ -117,23 +119,11 @@ contract BiconomySponsorshipPaymaster is emit FeeCollectorChanged(oldFeeCollector, _newFeeCollector, msg.sender); } - /** - * @dev Set a new unaccountedEPGasOverhead value. - * @param value The new value to be set as the unaccountedEPGasOverhead. - * @notice only to be called by the owner of the contract. - */ - function setPostopCost(uint48 value) external payable onlyOwner { - require(value <= 200_000, "Gas overhead too high"); - uint256 oldValue = postopCost; - postopCost = value; - emit PostopCostChanged(oldValue, value); - } - /** * @dev Override the default implementation. */ function deposit() external payable virtual override { - revert("Use depositFor() instead"); + revert UseDepositForInstead(); } /** @@ -155,15 +145,19 @@ contract BiconomySponsorshipPaymaster is function withdrawTo(address payable withdrawAddress, uint256 amount) external override nonReentrant { if (withdrawAddress == address(0)) revert CanNotWithdrawToZeroAddress(); uint256 currentBalance = paymasterIdBalances[msg.sender]; - require(amount <= currentBalance, "Sponsorship Paymaster: Insufficient funds to withdraw from gas tank"); + if (amount > currentBalance) { + revert InsufficientFundsInGasTank(); + } paymasterIdBalances[msg.sender] = currentBalance - amount; entryPoint.withdrawTo(withdrawAddress, amount); emit GasWithdrawn(msg.sender, withdrawAddress, amount); } - function withdrawEth(address payable recipient, uint256 amount) external onlyOwner { + function withdrawEth(address payable recipient, uint256 amount) external onlyOwner nonReentrant { (bool success,) = recipient.call{ value: amount }(""); - require(success, "withdraw failed"); + if (!success) { + revert WithdrawalFailed(); + } } /** @@ -225,11 +219,13 @@ contract BiconomySponsorshipPaymaster is bytes calldata signature ) { - paymasterId = address(bytes20(paymasterAndData[VALID_PND_OFFSET:VALID_PND_OFFSET + 20])); - validUntil = uint48(bytes6(paymasterAndData[VALID_PND_OFFSET + 20:VALID_PND_OFFSET + 26])); - validAfter = uint48(bytes6(paymasterAndData[VALID_PND_OFFSET + 26:VALID_PND_OFFSET + 32])); - priceMarkup = uint32(bytes4(paymasterAndData[VALID_PND_OFFSET + 32:VALID_PND_OFFSET + 36])); - signature = paymasterAndData[VALID_PND_OFFSET + 36:]; + unchecked { + paymasterId = address(bytes20(paymasterAndData[VALID_PND_OFFSET:VALID_PND_OFFSET + 20])); + validUntil = uint48(bytes6(paymasterAndData[VALID_PND_OFFSET + 20:VALID_PND_OFFSET + 26])); + validAfter = uint48(bytes6(paymasterAndData[VALID_PND_OFFSET + 26:VALID_PND_OFFSET + 32])); + priceMarkup = uint32(bytes4(paymasterAndData[VALID_PND_OFFSET + 32:VALID_PND_OFFSET + 36])); + signature = paymasterAndData[VALID_PND_OFFSET + 36:]; + } } /// @notice Performs post-operation tasks, such as deducting the sponsored gas cost from the paymasterId's balance @@ -252,14 +248,12 @@ contract BiconomySponsorshipPaymaster is (address paymasterId, uint32 dynamicMarkup, bytes32 userOpHash) = abi.decode(context, (address, uint32, bytes32)); - uint256 balToDeduct = actualGasCost + postopCost * actualUserOpFeePerGas; - - uint256 costIncludingPremium = (balToDeduct * dynamicMarkup) / PRICE_DENOMINATOR; + uint256 costIncludingPremium = (actualGasCost * dynamicMarkup) / PRICE_DENOMINATOR; // deduct with premium paymasterIdBalances[paymasterId] -= costIncludingPremium; - uint256 actualPremium = costIncludingPremium - balToDeduct; + uint256 actualPremium = costIncludingPremium - actualGasCost; // "collect" premium paymasterIdBalances[feeCollector] += actualPremium; @@ -294,10 +288,9 @@ contract BiconomySponsorshipPaymaster is //ECDSA library supports both 64 and 65-byte long signatures. // we only "require" it here so that the revert reason on invalid signature will be of "VerifyingPaymaster", and // not "ECDSA" - require( - signature.length == 64 || signature.length == 65, - "VerifyingPaymaster: invalid signature length in paymasterAndData" - ); + if(signature.length != 64 && signature.length != 65){ + revert InvalidSignatureLength(); + } bool validSig = verifyingSigner.isValidSignatureNow( ECDSA_solady.toEthSignedMessageHash(getHash(userOp, paymasterId, validUntil, validAfter, priceMarkup)), @@ -309,18 +302,19 @@ contract BiconomySponsorshipPaymaster is return ("", _packValidationData(true, validUntil, validAfter)); } - require(priceMarkup <= 2e6 && priceMarkup > 0, "Sponsorship Paymaster: Invalid markup %"); + if (priceMarkup > 2e6 || priceMarkup == 0) { + revert InvalidPriceMarkup(); + } uint256 maxFeePerGas = userOp.unpackMaxFeePerGas(); // Send 1e6 for No markup // Send between 0 and 1e6 for discount - uint256 effectiveCost = ((requiredPreFund + (postopCost * maxFeePerGas)) * priceMarkup) / PRICE_DENOMINATOR; + uint256 effectiveCost = (requiredPreFund * priceMarkup) / PRICE_DENOMINATOR; - require( - effectiveCost <= paymasterIdBalances[paymasterId], - "Sponsorship Paymaster: paymasterId does not have enough deposit" - ); + if (effectiveCost > paymasterIdBalances[paymasterId]) { + revert InsufficientFundsForPaymasterId(); + } context = abi.encode(paymasterId, priceMarkup, userOpHash); diff --git a/test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol b/test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol index 058e340..4f58bbe 100644 --- a/test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol +++ b/test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol @@ -26,7 +26,6 @@ contract TestSponsorshipPaymasterWithPremium is NexusTestBase { assertEq(address(testArtifact.entryPoint()), ENTRYPOINT_ADDRESS); assertEq(testArtifact.verifyingSigner(), PAYMASTER_SIGNER.addr); assertEq(testArtifact.feeCollector(), PAYMASTER_FEE_COLLECTOR.addr); - assertEq(testArtifact.postopCost(), 0 wei); } function test_CheckInitialPaymasterState() external view { @@ -34,7 +33,6 @@ contract TestSponsorshipPaymasterWithPremium is NexusTestBase { assertEq(address(bicoPaymaster.entryPoint()), ENTRYPOINT_ADDRESS); assertEq(bicoPaymaster.verifyingSigner(), PAYMASTER_SIGNER.addr); assertEq(bicoPaymaster.feeCollector(), PAYMASTER_FEE_COLLECTOR.addr); - assertEq(bicoPaymaster.postopCost(), 0 wei); } function test_OwnershipTransfer() external prankModifier(PAYMASTER_OWNER.addr) { @@ -121,7 +119,7 @@ contract TestSponsorshipPaymasterWithPremium is NexusTestBase { } function test_RevertIf_DepositCalled() external { - vm.expectRevert("Use depositFor() instead"); + vm.expectRevert(abi.encodeWithSelector(UseDepositForInstead.selector)); bicoPaymaster.deposit{ value: 1 ether }(); } @@ -146,7 +144,7 @@ contract TestSponsorshipPaymasterWithPremium is NexusTestBase { } function test_RevertIf_WithdrawToExceedsBalance() external prankModifier(DAPP_ACCOUNT.addr) { - vm.expectRevert("Sponsorship Paymaster: Insufficient funds to withdraw from gas tank"); + vm.expectRevert(abi.encodeWithSelector(InsufficientFundsInGasTank.selector)); bicoPaymaster.withdrawTo(payable(DAN_ADDRESS), 1 ether); } @@ -261,32 +259,10 @@ contract TestSponsorshipPaymasterWithPremium is NexusTestBase { function test_RevertIf_WithdrawEthExceedsBalance() external prankModifier(PAYMASTER_OWNER.addr) { uint256 ethAmount = 10 ether; - vm.expectRevert("withdraw failed"); + vm.expectRevert(abi.encodeWithSelector(WithdrawalFailed.selector)); bicoPaymaster.withdrawEth(payable(ALICE_ADDRESS), ethAmount); } - function test_SetPostopCost() external prankModifier(PAYMASTER_OWNER.addr) { - uint48 initialPostopCost = bicoPaymaster.postopCost(); - assertEq(initialPostopCost, 0 wei); - uint48 newPostopCost = initialPostopCost + 1 wei; - - vm.expectEmit(true, true, false, true, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.PostopCostChanged(initialPostopCost, newPostopCost); - bicoPaymaster.setPostopCost(newPostopCost); - - uint48 resultingPostopCost = bicoPaymaster.postopCost(); - assertEq(resultingPostopCost, newPostopCost); - } - - function test_RevertIf_SetPostopCostToHigh() external prankModifier(PAYMASTER_OWNER.addr) { - uint48 initialPostopCost = bicoPaymaster.postopCost(); - assertEq(initialPostopCost, 0 wei); - uint48 newPostopCost = initialPostopCost + 200_001 wei; - - vm.expectRevert("Gas overhead too high"); - bicoPaymaster.setPostopCost(newPostopCost); - } - function test_WithdrawErc20() external prankModifier(PAYMASTER_OWNER.addr) { MockToken token = new MockToken("Token", "TKN"); uint256 mintAmount = 10 * (10 ** token.decimals()); diff --git a/test/foundry/unit/fuzz/TestFuzz_TestSponsorshipPaymasterWithPremium.t.sol b/test/foundry/unit/fuzz/TestFuzz_TestSponsorshipPaymasterWithPremium.t.sol index 17e4676..5fb19db 100644 --- a/test/foundry/unit/fuzz/TestFuzz_TestSponsorshipPaymasterWithPremium.t.sol +++ b/test/foundry/unit/fuzz/TestFuzz_TestSponsorshipPaymasterWithPremium.t.sol @@ -7,7 +7,6 @@ import { IBiconomySponsorshipPaymaster } from "../../../../contracts/interfaces/ import { BiconomySponsorshipPaymaster } from "../../../../contracts/sponsorship/SponsorshipPaymasterWithPremium.sol"; import { MockToken } from "./../../../../lib/nexus/contracts/mocks/MockToken.sol"; - contract TestFuzz_SponsorshipPaymasterWithPremium is NexusTestBase { BiconomySponsorshipPaymaster public bicoPaymaster; @@ -79,20 +78,6 @@ contract TestFuzz_SponsorshipPaymasterWithPremium is NexusTestBase { assertEq(address(bicoPaymaster).balance, 0 ether); } - function testFuzz_SetPostopCost(uint48 value) external prankModifier(PAYMASTER_OWNER.addr) { - vm.assume(value <= 200_000 wei); - uint48 initialPostopCost = bicoPaymaster.postopCost(); - assertEq(initialPostopCost, 0 wei); - uint48 newPostopCost = value; - - vm.expectEmit(true, true, false, true, address(bicoPaymaster)); - emit IBiconomySponsorshipPaymaster.PostopCostChanged(initialPostopCost, newPostopCost); - bicoPaymaster.setPostopCost(newPostopCost); - - uint48 resultingPostopCost = bicoPaymaster.postopCost(); - assertEq(resultingPostopCost, newPostopCost); - } - function testFuzz_WithdrawErc20(address target, uint256 amount) external prankModifier(PAYMASTER_OWNER.addr) { vm.assume(target != address(0)); vm.assume(amount <= 1_000_000 * (10 ** 18));