Skip to content

Commit

Permalink
switched to custom errors and fixed postop gas calc
Browse files Browse the repository at this point in the history
  • Loading branch information
@ShivaanshK
ShivaanshK committed Jul 4, 2024
1 parent 38f9891 commit 95b996d
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 80 deletions.
30 changes: 30 additions & 0 deletions contracts/common/Errors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
*/
Expand Down
70 changes: 32 additions & 38 deletions contracts/sponsorship/SponsorshipPaymasterWithPremium.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}
Expand Down Expand Up @@ -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();
}

/**
Expand All @@ -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();
}
}

/**
Expand Down Expand Up @@ -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
Expand All @@ -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;

Expand Down Expand Up @@ -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)),
Expand All @@ -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);

Expand Down
30 changes: 3 additions & 27 deletions test/foundry/unit/concrete/TestSponsorshipPaymasterWithPremiumTest.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,13 @@ 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 {
assertEq(bicoPaymaster.owner(), PAYMASTER_OWNER.addr);
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) {
Expand Down Expand Up @@ -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 }();
}

Expand All @@ -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);
}

Expand Down Expand Up @@ -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());
Expand Down
15 changes: 0 additions & 15 deletions test/foundry/unit/fuzz/TestFuzz_TestSponsorshipPaymasterWithPremium.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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));
Expand Down

0 comments on commit 95b996d

Please sign in to comment.