Skip to content

Commit

Permalink
fix: remediations - test update
Browse files Browse the repository at this point in the history
  • Loading branch information
@livingrockrises
livingrockrises committed Oct 8, 2024
1 parent 1d9364e commit 5b54caa
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 26 deletions.
5 changes: 5 additions & 0 deletions contracts/common/BiconomySponsorshipPaymasterErrors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,9 @@ contract BiconomySponsorshipPaymasterErrors {
* @notice Throws when trying unaccountedGas is too high
*/
error UnaccountedGasTooHigh();

/**
* @notice Throws when postOp gas limit is too low
*/
error PostOpGasLimitTooLow();
}
51 changes: 32 additions & 19 deletions contracts/sponsorship/BiconomySponsorshipPaymaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ contract BiconomySponsorshipPaymaster is
// Offset in PaymasterAndData to get to PAYMASTER_ID_OFFSET
uint256 private constant PAYMASTER_ID_OFFSET = PAYMASTER_DATA_OFFSET;
// Limit for unaccounted gas cost
uint256 private constant UNACCOUNTED_GAS_LIMIT = 50_000;
// Review cap
uint256 private constant UNACCOUNTED_GAS_LIMIT = 100_000;

mapping(address => uint256) public paymasterIdBalances;

Expand Down Expand Up @@ -256,26 +257,30 @@ contract BiconomySponsorshipPaymaster is
override
{
unchecked {
(address paymasterId, uint32 priceMarkup, bytes32 userOpHash) =
abi.decode(context, (address, uint32, bytes32));
(address paymasterId, uint32 priceMarkup, bytes32 userOpHash, uint256 prechargedAmount, uint256 unaccountedGasCatched) =
abi.decode(context, (address, uint32, bytes32, uint256, uint256));

// Include unaccountedGas since EP doesn't include this in actualGasCost
// unaccountedGas = postOpGas + EP overhead gas + estimated penalty
actualGasCost = actualGasCost + (unaccountedGas * actualUserOpFeePerGas);
actualGasCost = actualGasCost + (unaccountedGasCatched * actualUserOpFeePerGas);
// Apply the price markup
uint256 adjustedGasCost = (actualGasCost * priceMarkup) / PRICE_DENOMINATOR;

// Deduct the adjusted cost
paymasterIdBalances[paymasterId] -= adjustedGasCost;

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);
if(prechargedAmount - adjustedGasCost > 0) {
// If overcharged refund the excess
paymasterIdBalances[paymasterId] += (prechargedAmount -adjustedGasCost);
}

// 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);
}
}
Expand All @@ -296,7 +301,6 @@ contract BiconomySponsorshipPaymaster is
uint256 requiredPreFund
)
internal
view
override
returns (bytes memory context, uint256 validationData)
{
Expand All @@ -309,6 +313,13 @@ contract BiconomySponsorshipPaymaster is
revert InvalidSignatureLength();
}

uint256 unaccountedGasCatched = unaccountedGas;

// WIP
// if(unaccountedGasCatched >= userOp.unpackPostOpGasLimit()) {
// revert PostOpGasLimitTooLow();
// }

bool validSig = verifyingSigner.isValidSignatureNow(
ECDSA_solady.toEthSignedMessageHash(getHash(userOp, paymasterId, validUntil, validAfter, priceMarkup)),
signature
Expand All @@ -319,19 +330,21 @@ contract BiconomySponsorshipPaymaster is
return ("", _packValidationData(true, validUntil, validAfter));
}

if (priceMarkup > 2e6 || priceMarkup == 0) {
// Send 1e6 for No markup
if (priceMarkup > 2e6 || priceMarkup < 1e6) {
revert InvalidPriceMarkup();
}

// Send 1e6 for No markup
// Send between 0 and 1e6 for discount
uint256 effectiveCost = (requiredPreFund * priceMarkup) / PRICE_DENOMINATOR;
// Deduct the max gas cost.
uint256 effectiveCost = (requiredPreFund + unaccountedGasCatched * userOp.unpackMaxFeePerGas()) * priceMarkup / PRICE_DENOMINATOR;

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

context = abi.encode(paymasterId, priceMarkup, userOpHash);
paymasterIdBalances[paymasterId] -= effectiveCost;

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

//no need for other on-chain validation: entire UserOp should have been checked
// by the external service prior to signing it.
Expand Down
14 changes: 7 additions & 7 deletions test/unit/concrete/TestSponsorshipPaymaster.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,19 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
setupPaymasterTestEnvironment();
// Deploy Sponsorship Paymaster
bicoPaymaster = new BiconomySponsorshipPaymaster(
PAYMASTER_OWNER.addr, ENTRYPOINT, PAYMASTER_SIGNER.addr, PAYMASTER_FEE_COLLECTOR.addr, 7e3
PAYMASTER_OWNER.addr, ENTRYPOINT, PAYMASTER_SIGNER.addr, PAYMASTER_FEE_COLLECTOR.addr, 7e4
);
}

function test_Deploy() external {
BiconomySponsorshipPaymaster testArtifact = new BiconomySponsorshipPaymaster(
PAYMASTER_OWNER.addr, ENTRYPOINT, PAYMASTER_SIGNER.addr, PAYMASTER_FEE_COLLECTOR.addr, 7e3
PAYMASTER_OWNER.addr, ENTRYPOINT, PAYMASTER_SIGNER.addr, PAYMASTER_FEE_COLLECTOR.addr, 7e4
);
assertEq(testArtifact.owner(), PAYMASTER_OWNER.addr);
assertEq(address(testArtifact.entryPoint()), ENTRYPOINT_ADDRESS);
assertEq(testArtifact.verifyingSigner(), PAYMASTER_SIGNER.addr);
assertEq(testArtifact.feeCollector(), PAYMASTER_FEE_COLLECTOR.addr);
assertEq(testArtifact.unaccountedGas(), 7e3);
assertEq(testArtifact.unaccountedGas(), 7e4);
}

function test_RevertIf_DeployWithSignerSetToZero() external {
Expand Down Expand Up @@ -56,7 +56,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
function test_RevertIf_DeployWithUnaccountedGasCostTooHigh() external {
vm.expectRevert(abi.encodeWithSelector(UnaccountedGasTooHigh.selector));
new BiconomySponsorshipPaymaster(
PAYMASTER_OWNER.addr, ENTRYPOINT, PAYMASTER_SIGNER.addr, PAYMASTER_FEE_COLLECTOR.addr, 50_001
PAYMASTER_OWNER.addr, ENTRYPOINT, PAYMASTER_SIGNER.addr, PAYMASTER_FEE_COLLECTOR.addr, 100_001
);
}

Expand All @@ -65,7 +65,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
assertEq(address(bicoPaymaster.entryPoint()), ENTRYPOINT_ADDRESS);
assertEq(bicoPaymaster.verifyingSigner(), PAYMASTER_SIGNER.addr);
assertEq(bicoPaymaster.feeCollector(), PAYMASTER_FEE_COLLECTOR.addr);
assertEq(bicoPaymaster.unaccountedGas(), 7e3);
assertEq(bicoPaymaster.unaccountedGas(), 7e4);
}

function test_OwnershipTransfer() external prankModifier(PAYMASTER_OWNER.addr) {
Expand Down Expand Up @@ -130,7 +130,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {

function test_SetUnaccountedGas() external prankModifier(PAYMASTER_OWNER.addr) {
uint256 initialUnaccountedGas = bicoPaymaster.unaccountedGas();
uint256 newUnaccountedGas = 5000;
uint256 newUnaccountedGas = 80000;

vm.expectEmit(true, true, false, true, address(bicoPaymaster));
emit IBiconomySponsorshipPaymaster.UnaccountedGasChanged(initialUnaccountedGas, newUnaccountedGas);
Expand All @@ -141,7 +141,7 @@ contract TestSponsorshipPaymasterWithPriceMarkup is TestBase {
}

function test_RevertIf_SetUnaccountedGasToHigh() external prankModifier(PAYMASTER_OWNER.addr) {
uint16 newUnaccountedGas = 50_001;
uint256 newUnaccountedGas = 100001;
vm.expectRevert(abi.encodeWithSelector(UnaccountedGasTooHigh.selector));
bicoPaymaster.setUnaccountedGas(newUnaccountedGas);
}
Expand Down

0 comments on commit 5b54caa

Please sign in to comment.