fix:remediations chainlight 001, 002, 004

contracts/common/BiconomySponsorshipPaymasterErrors.sol

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

contracts/sponsorship/BiconomySponsorshipPaymaster.sol

@@ -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;
 
@@ -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);
         }
     }
@@ -296,7 +301,6 @@ contract BiconomySponsorshipPaymaster is
         uint256 requiredPreFund
     )
         internal
-        view
         override
         returns (bytes memory context, uint256 validationData)
     {
@@ -309,6 +313,14 @@ contract BiconomySponsorshipPaymaster is
             revert InvalidSignatureLength();
         }
 
+        // Note: may not need to cache and forward in the context
+        uint256 unaccountedGasCatched = unaccountedGas;
+
+
+        if(unaccountedGasCatched >= userOp.unpackPostOpGasLimit()) {
+            revert PostOpGasLimitTooLow();
+        } 
+
         bool validSig = verifyingSigner.isValidSignatureNow(
             ECDSA_solady.toEthSignedMessageHash(getHash(userOp, paymasterId, validUntil, validAfter, priceMarkup)),
             signature
@@ -319,19 +331,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.

test/base/TestBase.sol

@@ -3,6 +3,7 @@ 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";
@@ -130,7 +131,7 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors {
         userOp = buildUserOpWithCalldata(sender, "", address(VALIDATOR_MODULE));
 
         (userOp.paymasterAndData,) = generateAndSignPaymasterData(
-            userOp, PAYMASTER_SIGNER, paymaster, 3e6, 3e6, DAPP_ACCOUNT.addr, validUntil, validAfter, priceMarkup
+            userOp, PAYMASTER_SIGNER, paymaster, 3e6, 8e3, DAPP_ACCOUNT.addr, validUntil, validAfter, priceMarkup
         );
         userOp.signature = signUserOp(sender, userOp);
 
@@ -139,11 +140,21 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors {
         (, uint256 postopGasUsed, uint256 validationGasLimit, uint256 postopGasLimit) =
             estimatePaymasterGasCosts(paymaster, userOp, 5e4);
 
+        // console2.log("postOpGasUsed");
+        // console2.logUint(postopGasUsed); 
+
+        // uint256 prevValUnaccountedGas = paymaster.unaccountedGas();
+        // console2.logUint(prevValUnaccountedGas);
+
+        // Below may not be needed if unaccountedGas is set correctly
         vm.startPrank(paymaster.owner());
         // Set unaccounted gas to be gas used in postop + 1000 for EP overhead and penalty
-        paymaster.setUnaccountedGas(uint16(postopGasUsed + 1000));
+        paymaster.setUnaccountedGas(postopGasUsed + 500);
         vm.stopPrank();
 
+        // uint256 currentValUnaccountedGas = paymaster.unaccountedGas();
+        // console2.logUint(currentValUnaccountedGas);
+
         // Ammend the userop to have new gas limits and signature
         userOp.accountGasLimits = bytes32(abi.encodePacked(uint128(verificationGasLimit), uint128(callGasLimit)));
         (userOp.paymasterAndData,) = generateAndSignPaymasterData(

test/unit/concrete/TestSponsorshipPaymaster.t.sol

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