Skip to content

Commit

Permalink
Merge pull request #26 from bcnmy/fix/chainlight-006-unused-gas-penalty
Browse files Browse the repository at this point in the history 
Fix/chainlight 006 Unused Gas Penalty
  • Loading branch information
@livingrockrises
livingrockrises authored Oct 18, 2024
2 parents d2c8096 + 2ea2a95 commit 6c81989
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 39 deletions.
3 changes: 1 addition & 2 deletions contracts/interfaces/IBiconomySponsorshipPaymaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ interface IBiconomySponsorshipPaymaster {
event FeeCollectorChanged(address indexed oldFeeCollector, address indexed newFeeCollector, address indexed actor);
event GasDeposited(address indexed paymasterId, uint256 indexed value);
event GasWithdrawn(address indexed paymasterId, address indexed to, uint256 indexed value);
event GasBalanceDeducted(address indexed paymasterId, uint256 indexed charge, bytes32 indexed userOpHash);
event PriceMarkupCollected(address indexed paymasterId, uint256 indexed priceMarkup);
event GasBalanceDeducted(address indexed paymasterId, uint256 actualGasCost, uint256 indexed adjustedGasCost, bytes32 indexed userOpHash);

Check warning on line 14 in contracts/interfaces/IBiconomySponsorshipPaymaster.sol

View workflow job for this annotation

GitHub Actions / Lint sources 

Line length must be no more than 120 but current length is 142
event Received(address indexed sender, uint256 value);
event TokensWithdrawn(address indexed token, address indexed to, uint256 indexed amount, address actor);

Expand Down
28 changes: 14 additions & 14 deletions contracts/sponsorship/BiconomySponsorshipPaymaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,18 +274,12 @@ contract BiconomySponsorshipPaymaster is
// If overcharged refund the excess
paymasterIdBalances[paymasterId] += (prechargedAmount - adjustedGasCost);
}

// Add priceMarkup to fee collector balance
paymasterIdBalances[feeCollector] += adjustedGasCost - actualGasCost;

// Should always be true
// if (adjustedGasCost > actualGasCost) {
// Apply priceMarkup to fee collector balance
uint256 premium = adjustedGasCost - actualGasCost;
paymasterIdBalances[feeCollector] += premium;
// Review: if we should emit adjustedGasCost as well
emit PriceMarkupCollected(paymasterId, premium);
// }

// Review: emit min required information
emit GasBalanceDeducted(paymasterId, adjustedGasCost, userOpHash);
// premium = adjustedGasCost - actualGasCost => do not need to emit it explicitly
emit GasBalanceDeducted(paymasterId, actualGasCost, adjustedGasCost, userOpHash);
}
}

Expand Down Expand Up @@ -337,15 +331,21 @@ contract BiconomySponsorshipPaymaster is
revert InvalidPriceMarkup();
}

// callGasLimit + paymasterPostOpGas
uint256 maxPenalty = (
uint128(uint256(userOp.accountGasLimits)) +
uint128(bytes16(userOp.paymasterAndData[_PAYMASTER_POSTOP_GAS_OFFSET : _PAYMASTER_DATA_OFFSET]))
) * 10 * userOp.unpackMaxFeePerGas() / 100;

// Deduct the max gas cost.
uint256 effectiveCost =
((requiredPreFund + unaccountedGas * userOp.unpackMaxFeePerGas()) * priceMarkup) / _PRICE_DENOMINATOR;
((requiredPreFund + unaccountedGas * userOp.unpackMaxFeePerGas()) * priceMarkup / _PRICE_DENOMINATOR);

if (effectiveCost > paymasterIdBalances[paymasterId]) {
if (effectiveCost + maxPenalty > paymasterIdBalances[paymasterId]) {
revert InsufficientFundsForPaymasterId();
}

paymasterIdBalances[paymasterId] -= effectiveCost;
paymasterIdBalances[paymasterId] -= (effectiveCost + maxPenalty);

context = abi.encode(paymasterId, priceMarkup, userOpHash, effectiveCost);

Expand Down
37 changes: 29 additions & 8 deletions test/base/TestBase.sol
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;

import { Test } from "forge-std/Test.sol";
import { Vm } from "forge-std/Vm.sol";
import { console2 } from "forge-std/console2.sol";

import "solady/utils/ECDSA.sol";
import "./TestHelper.sol";
import "account-abstraction/core/UserOperationLib.sol";

import { IAccount } from "account-abstraction/interfaces/IAccount.sol";
import { Exec } from "account-abstraction/utils/Exec.sol";
Expand All @@ -26,17 +26,21 @@ import {
} from "../../../contracts/token/BiconomyTokenPaymaster.sol";

abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors {
address constant ENTRYPOINT_ADDRESS = address(0x0000000071727De22E5E9d8BAf0edAc6f37da032);

address constant WRAPPED_NATIVE_ADDRESS = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
using UserOperationLib for PackedUserOperation;

address constant ENTRYPOINT_ADDRESS = address(0x0000000071727De22E5E9d8BAf0edAc6f37da032);
address constant WRAPPED_NATIVE_ADDRESS = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
address constant SWAP_ROUTER_ADDRESS = address(0xE592427A0AEce92De3Edee1F18E0157C05861564);

Vm.Wallet internal PAYMASTER_OWNER;
Vm.Wallet internal PAYMASTER_SIGNER;
Vm.Wallet internal PAYMASTER_FEE_COLLECTOR;
Vm.Wallet internal DAPP_ACCOUNT;

uint256 internal constant _PAYMASTER_POSTOP_GAS_OFFSET = UserOperationLib.PAYMASTER_POSTOP_GAS_OFFSET;
uint256 internal constant _PAYMASTER_DATA_OFFSET = UserOperationLib.PAYMASTER_DATA_OFFSET;

struct PaymasterData {
uint128 validationGasLimit;
uint128 postOpGasLimit;
Expand Down Expand Up @@ -310,7 +314,8 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors {
BiconomySponsorshipPaymaster paymaster,
uint256 initialDappPaymasterBalance,
uint256 initialFeeCollectorBalance,
uint32 priceMarkup
uint32 priceMarkup,
uint256 maxPenalty
)
internal
view
Expand All @@ -320,38 +325,54 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors {
uint256 resultingFeeCollectorPaymasterBalance = paymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr);

uint256 totalGasFeesCharged = initialDappPaymasterBalance - resultingDappPaymasterBalance;
uint256 accountableGasFeesCharged = totalGasFeesCharged - maxPenalty;

expectedPriceMarkup = totalGasFeesCharged - ((totalGasFeesCharged * 1e6) / priceMarkup);
expectedPriceMarkup = accountableGasFeesCharged - ((accountableGasFeesCharged * 1e6) / priceMarkup);
actualPriceMarkup = resultingFeeCollectorPaymasterBalance - initialFeeCollectorBalance;
}

function getMaxPenalty(PackedUserOperation calldata userOp) public view returns (uint256) {
return (
uint128(uint256(userOp.accountGasLimits)) +
uint128(bytes16(userOp.paymasterAndData[_PAYMASTER_POSTOP_GAS_OFFSET : _PAYMASTER_DATA_OFFSET]))
) * 10 * userOp.unpackMaxFeePerGas() / 100;
}

// Note: can pack values into one struct
function calculateAndAssertAdjustments(
BiconomySponsorshipPaymaster bicoPaymaster,
uint256 initialDappPaymasterBalance,
uint256 initialFeeCollectorBalance,
uint256 initialBundlerBalance,
uint256 initialPaymasterEpBalance,
uint32 priceMarkup
uint32 priceMarkup,
uint256 maxPenalty
)
internal
view
{
(uint256 expectedPriceMarkup, uint256 actualPriceMarkup) =
getPriceMarkups(bicoPaymaster, initialDappPaymasterBalance, initialFeeCollectorBalance, priceMarkup);
getPriceMarkups(bicoPaymaster, initialDappPaymasterBalance, initialFeeCollectorBalance, priceMarkup, maxPenalty);
uint256 totalGasFeePaid = BUNDLER.addr.balance - initialBundlerBalance;
uint256 gasPaidByDapp = initialDappPaymasterBalance - bicoPaymaster.getBalance(DAPP_ACCOUNT.addr);

console2.log("1");
// Assert that what paymaster paid is the same as what the bundler received
assertEq(totalGasFeePaid, initialPaymasterEpBalance - bicoPaymaster.getDeposit());

console2.log("2");
// Assert that adjustment collected (if any) is correct
assertEq(expectedPriceMarkup, actualPriceMarkup);

console2.log("3");
// Gas paid by dapp is higher than paymaster
// Guarantees that EP always has sufficient deposit to pay back dapps
assertGt(gasPaidByDapp, BUNDLER.addr.balance - initialBundlerBalance);

console2.log("4");
// Ensure that max 2% difference between total gas paid + the adjustment premium and gas paid by dapp (from
// paymaster)
assertApproxEqRel(totalGasFeePaid + actualPriceMarkup, gasPaidByDapp, 0.02e18);
assertApproxEqRel(totalGasFeePaid + actualPriceMarkup + maxPenalty, gasPaidByDapp, 0.02e18);
}

function _toSingletonArray(address addr) internal pure returns (address[] memory) {
Expand Down
16 changes: 8 additions & 8 deletions test/unit/concrete/TestSponsorshipPaymaster.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr);

// submit userops
vm.expectEmit(true, false, true, true, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash);
vm.expectEmit(true, false, false, false, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash);
startPrank(BUNDLER.addr);
ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr));
stopPrank();
Expand All @@ -236,7 +236,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
initialFeeCollectorBalance,
initialBundlerBalance,
initialPaymasterEpBalance,
1e6
1e6, // priceMarkup
this.getMaxPenalty(userOp)
);
}

Expand All @@ -258,10 +259,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr);

// submit userops
vm.expectEmit(true, false, false, true, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.PriceMarkupCollected(DAPP_ACCOUNT.addr, 0);
vm.expectEmit(true, false, true, true, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash);
vm.expectEmit(true, false, false, false, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash);

startPrank(BUNDLER.addr);
ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr));
Expand All @@ -274,7 +273,8 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
initialFeeCollectorBalance,
initialBundlerBalance,
initialPaymasterEpBalance,
1_100_000
1_100_000, //price markup, +10% paymaster fee
this.getMaxPenalty(userOp)
);
}

Expand Down
12 changes: 5 additions & 7 deletions test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase {
}

// Review: fuzz with high markeup and current set values.
function skip_testFuzz_ValidatePaymasterAndPostOpWithPriceMarkup(uint32 priceMarkup) external {
function testFuzz_ValidatePaymasterAndPostOpWithPriceMarkup(uint32 priceMarkup) external {
vm.assume(priceMarkup <= 2e6 && priceMarkup > 1e6);
bicoPaymaster.depositFor{ value: 10 ether }(DAPP_ACCOUNT.addr);

Expand All @@ -111,11 +111,8 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase {
uint256 initialDappPaymasterBalance = bicoPaymaster.getBalance(DAPP_ACCOUNT.addr);
uint256 initialFeeCollectorBalance = bicoPaymaster.getBalance(PAYMASTER_FEE_COLLECTOR.addr);

// submit userops
vm.expectEmit(true, false, false, true, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.PriceMarkupCollected(DAPP_ACCOUNT.addr, 0);
vm.expectEmit(true, false, true, true, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, userOpHash);
vm.expectEmit(true, false, false, false, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.GasBalanceDeducted(DAPP_ACCOUNT.addr, 0, 0, userOpHash);

startPrank(BUNDLER.addr);
ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr));
Expand All @@ -128,7 +125,8 @@ contract TestFuzz_SponsorshipPaymasterWithPriceMarkup is TestBase {
initialFeeCollectorBalance,
initialBundlerBalance,
initialPaymasterEpBalance,
priceMarkup
priceMarkup,
this.getMaxPenalty(userOp)
);
}

Expand Down

0 comments on commit 6c81989

Please sign in to comment.