Fix/chainlight remediations

contracts/interfaces/paymasters/IBiconomyTokenPaymaster.sol

@@ -32,6 +32,8 @@ interface IBiconomyTokenPaymaster {
      */
     event FeeReceiverChanged(address indexed _oldfeeReceiver, address indexed _newfeeReceiver, address indexed _actor);
 
+    event TokensWithdrawn(address indexed _token, address indexed _to, uint256 indexed _amount, address actor);
+
     /**
      * Designed to enable tracking how much fees were charged from the sender and in which ERC20 token
      * More information can be emitted like exchangeRate used, what was the source of exchangeRate etc

contracts/token/BiconomyTokenPaymaster.sol

@@ -14,6 +14,7 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import "@account-abstraction/contracts/core/Helpers.sol" as Helpers;
 import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
 import "../utils/SafeTransferLib.sol";
+import {MathLib} from "../libs/MathLib.sol";
 import {TokenPaymasterErrors} from "./TokenPaymasterErrors.sol";
 import "@openzeppelin/contracts/utils/Address.sol";
 import {OracleAggregator} from "./oracles/OracleAggregator.sol";
@@ -77,6 +78,10 @@ contract BiconomyTokenPaymaster is
         }
     }
 
+    receive() external payable {
+        emit Received(msg.sender, msg.value);
+    }
+
     /**
      * @dev Set a new verifying signer address.
      * Can only be called by the owner of the contract.
@@ -178,11 +183,12 @@ contract BiconomyTokenPaymaster is
         onlyOwner
         nonReentrant
     {
-        if (token.length != amount.length) {
+        uint256 tokLen = token.length;
+        if (tokLen != amount.length) {
             revert TokensAndAmountsLengthMismatch();
         }
         unchecked {
-            for (uint256 i; i < token.length;) {
+            for (uint256 i; i < tokLen;) {
                 _withdrawERC20(token[i], target, amount[i]);
                 ++i;
             }
@@ -196,7 +202,8 @@ contract BiconomyTokenPaymaster is
      */
     function withdrawMultipleERC20Full(IERC20[] calldata token, address target) public payable onlyOwner nonReentrant {
         unchecked {
-            for (uint256 i; i < token.length;) {
+            uint256 tokLen = token.length;
+            for (uint256 i; i < tokLen;) {
                 uint256 amount = token[i].balanceOf(address(this));
                 _withdrawERC20(token[i], target, amount);
                 ++i;
@@ -283,71 +290,6 @@ contract BiconomyTokenPaymaster is
         signature = paymasterAndData[VALID_PND_OFFSET + 53:];
     }
 
-    function _getRequiredPrefund(UserOperation calldata userOp) internal view returns (uint256 requiredPrefund) {
-        unchecked {
-            uint256 requiredGas =
-                userOp.callGasLimit + userOp.verificationGasLimit + userOp.preVerificationGas + unaccountedEPGasOverhead;
-
-            requiredPrefund = requiredGas * userOp.maxFeePerGas;
-        }
-    }
-
-    /**
-     * @dev Verify that an external signer signed the paymaster data of a user operation.
-     * The paymaster data is expected to be the paymaster address, request data and a signature over the entire request parameters.
-     * paymasterAndData: hexConcat([paymasterAddress, priceSource, abi.encode(validUntil, validAfter, feeToken, exchangeRate, priceMarkup), signature])
-     * @param userOp The UserOperation struct that represents the current user operation.
-     * userOpHash The hash of the UserOperation struct.
-     * @param requiredPreFund The required amount of pre-funding for the paymaster.
-     * @return context A context string returned by the entry point after successful validation.
-     * @return validationData An integer returned by the entry point after successful validation.
-     */
-    function _validatePaymasterUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 requiredPreFund)
-        internal
-        view
-        override
-        returns (bytes memory context, uint256 validationData)
-    {
-        (requiredPreFund);
-
-        (
-            ExchangeRateSource priceSource,
-            uint48 validUntil,
-            uint48 validAfter,
-            address feeToken,
-            uint128 exchangeRate,
-            uint32 priceMarkup,
-            bytes calldata signature
-        ) = parsePaymasterAndData(userOp.paymasterAndData);
-
-        bytes32 _hash = getHash(userOp, priceSource, validUntil, validAfter, feeToken, exchangeRate, priceMarkup)
-            .toEthSignedMessageHash();
-
-        //don't revert on signature failure: return SIG_VALIDATION_FAILED
-        if (verifyingSigner != _hash.recover(signature)) {
-            // empty context and sigFailed true
-            return (context, Helpers._packValidationData(true, validUntil, validAfter));
-        }
-
-        address account = userOp.getSender();
-
-        // This model assumes irrespective of priceSource exchangeRate is always sent from outside
-        // for below checks you would either need maxCost or some exchangeRate
-
-        uint256 btpmRequiredPrefund = _getRequiredPrefund(userOp);
-
-        uint256 tokenRequiredPreFund = (btpmRequiredPrefund * exchangeRate) / 10 ** 18;
-        require(priceMarkup <= 2e6, "BTPM: price markup percentage too high");
-        require(
-            IERC20(feeToken).balanceOf(account) >= ((tokenRequiredPreFund * priceMarkup) / PRICE_DENOMINATOR),
-            "BTPM: account does not have enough token balance"
-        );
-
-        context = abi.encode(account, feeToken, priceSource, exchangeRate, priceMarkup, userOpHash);
-
-        return (context, Helpers._packValidationData(false, validUntil, validAfter));
-    }
-
     /**
      * @dev Executes the paymaster's payment conditions
      * @param mode tells whether the op succeeded, reverted, or if the op succeeded but cause the postOp to revert
@@ -360,11 +302,13 @@ contract BiconomyTokenPaymaster is
         ExchangeRateSource priceSource;
         uint128 exchangeRate;
         uint32 priceMarkup;
+        uint256 maxFeePerGas;
+        uint256 maxPriorityFeePerGas;
         bytes32 userOpHash;
         assembly ("memory-safe") {
             let offset := context.offset
 
-            account := calldataload(context.offset)
+            account := calldataload(offset)
             offset := add(offset, 0x20)
 
             feeToken := calldataload(offset)
@@ -379,6 +323,12 @@ contract BiconomyTokenPaymaster is
             priceMarkup := calldataload(offset)
             offset := add(offset, 0x20)
 
+            maxFeePerGas := calldataload(offset)
+            offset := add(offset, 0x20)
+
+            maxPriorityFeePerGas := calldataload(offset)
+            offset := add(offset, 0x20)
+
             userOpHash := calldataload(offset)
         }
 
@@ -389,11 +339,16 @@ contract BiconomyTokenPaymaster is
             if (result != 0) effectiveExchangeRate = result;
         }
 
+        uint256 effectiveGasPrice = getGasPrice(
+            maxFeePerGas,
+            maxPriorityFeePerGas
+        );
+
         // We could either touch the state for BASEFEE and calculate based on maxPriorityFee passed (to be added in context along with maxFeePerGas) or just use tx.gasprice
         uint256 charge; // Final amount to be charged from user account
         {
             uint256 actualTokenCost =
-                ((actualGasCost + (unaccountedEPGasOverhead * tx.gasprice)) * effectiveExchangeRate) / 1e18;
+                ((actualGasCost + (unaccountedEPGasOverhead * effectiveGasPrice)) * effectiveExchangeRate) / 1e18;
             charge = ((actualTokenCost * priceMarkup) / PRICE_DENOMINATOR);
         }
 
@@ -423,12 +378,90 @@ contract BiconomyTokenPaymaster is
         }
     }
 
+    function _getRequiredPrefund(UserOperation calldata userOp) internal view returns (uint256 requiredPrefund) {
+        unchecked {
+            uint256 requiredGas =
+                userOp.callGasLimit + userOp.verificationGasLimit + userOp.preVerificationGas + unaccountedEPGasOverhead;
+
+            requiredPrefund = requiredGas * userOp.maxFeePerGas;
+        }
+    }
+
+    // Note: do not use this in validation phase
+    function getGasPrice(
+        uint256 maxFeePerGas,
+        uint256 maxPriorityFeePerGas
+    ) internal view returns (uint256) {
+        if (maxFeePerGas == maxPriorityFeePerGas) {
+            //legacy mode (for networks that don't support basefee opcode)
+            return maxFeePerGas;
+        }
+        return
+            MathLib.minuint256(
+                maxFeePerGas,
+                maxPriorityFeePerGas + block.basefee
+            );
+    }
+
+    /**
+     * @dev Verify that an external signer signed the paymaster data of a user operation.
+     * The paymaster data is expected to be the paymaster address, request data and a signature over the entire request parameters.
+     * paymasterAndData: hexConcat([paymasterAddress, priceSource, abi.encode(validUntil, validAfter, feeToken, exchangeRate, priceMarkup), signature])
+     * @param userOp The UserOperation struct that represents the current user operation.
+     * userOpHash The hash of the UserOperation struct.
+     * @param requiredPreFund The required amount of pre-funding for the paymaster.
+     * @return context A context string returned by the entry point after successful validation.
+     * @return validationData An integer returned by the entry point after successful validation.
+     */
+    function _validatePaymasterUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 requiredPreFund)
+        internal
+        view
+        override
+        returns (bytes memory context, uint256 validationData)
+    {
+        (requiredPreFund);
+
+        (
+            ExchangeRateSource priceSource,
+            uint48 validUntil,
+            uint48 validAfter,
+            address feeToken,
+            uint128 exchangeRate,
+            uint32 priceMarkup,
+            bytes calldata signature
+        ) = parsePaymasterAndData(userOp.paymasterAndData);
+
+        bytes32 _hash = getHash(userOp, priceSource, validUntil, validAfter, feeToken, exchangeRate, priceMarkup)
+            .toEthSignedMessageHash();
+
+        //don't revert on signature failure: return SIG_VALIDATION_FAILED
+        if (verifyingSigner != _hash.recover(signature)) {
+            // empty context and sigFailed true
+            return (context, Helpers._packValidationData(true, validUntil, validAfter));
+        }
+
+        address account = userOp.getSender();
+
+        // This model assumes irrespective of priceSource exchangeRate is always sent from outside
+        // for below checks you would either need maxCost or some exchangeRate
+
+        uint256 btpmRequiredPrefund = _getRequiredPrefund(userOp);
+
+        uint256 tokenRequiredPreFund = (btpmRequiredPrefund * exchangeRate) / 10 ** 18;
+        require(priceMarkup <= 2e6, "BTPM: price markup percentage too high");
+        require(
+            IERC20(feeToken).balanceOf(account) >= ((tokenRequiredPreFund * priceMarkup) / PRICE_DENOMINATOR),
+            "BTPM: account does not have enough token balance"
+        );
+
+        context = abi.encode(account, feeToken, priceSource, exchangeRate, priceMarkup, userOp.maxFeePerGas, userOp.maxPriorityFeePerGas, userOpHash);
+
+        return (context, Helpers._packValidationData(false, validUntil, validAfter));
+    }
+
     function _withdrawERC20(IERC20 token, address target, uint256 amount) private {
         if (target == address(0)) revert CanNotWithdrawToZeroAddress();
         SafeTransferLib.safeTransfer(address(token), target, amount);
-    }
-
-    receive() external payable {
-        emit Received(msg.sender, msg.value);
+        emit TokensWithdrawn(address(token), target, amount, msg.sender);
     }
 }

contracts/token/oracles/IOracleAggregator.sol

@@ -2,18 +2,19 @@
 pragma solidity ^0.8.23;
 
 interface IOracleAggregator {
-    error MismatchInBaseAndQuoteDecimals();
-    error InvalidPriceFromRound();
-    error LatestRoundIncomplete();
-    error PriceFeedStale();
-    error OracleAddressCannotBeZero();
-
     struct TokenInfo {
         uint8 tokenDecimals;
         address tokenOracle;
         address nativeOracle;
         bool isDerivedFeed;
+        uint24 priceUpdateThreshold;
     }
 
+    error MismatchInBaseAndQuoteDecimals();
+    error InvalidPriceFromRound();
+    error LatestRoundIncomplete();
+    error PriceFeedStale();
+    error OracleAddressCannotBeZero();
+
     function getTokenValueOfOneNativeToken(address _token) external view returns (uint128 exchangeRate);
 }

contracts/token/oracles/OracleAggregator.sol

@@ -17,7 +17,8 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
         uint8 tokenDecimals,
         address tokenOracle,
         address nativeOracle,
-        bool isDerivedFeed
+        bool isDerivedFeed,
+        uint24 priceUpdateThreshold
     ) external onlyOwner {
         if (tokenOracle == address(0)) revert OracleAddressCannotBeZero();
         if (nativeOracle == address(0)) revert OracleAddressCannotBeZero();
@@ -29,6 +30,7 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
         tokensInfo[token].nativeOracle = nativeOracle;
         tokensInfo[token].tokenDecimals = tokenDecimals;
         tokensInfo[token].isDerivedFeed = isDerivedFeed;
+        tokensInfo[token].priceUpdateThreshold = priceUpdateThreshold;
     }
 
     /**
@@ -52,16 +54,17 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
     {
         TokenInfo storage tokenInfo = tokensInfo[token];
         tokenDecimals = tokenInfo.tokenDecimals;
+        uint24 priceUpdateThreshold = tokenInfo.priceUpdateThreshold;
 
         if (tokenInfo.isDerivedFeed) {
-            (uint256 price1, bool isError1) = fetchPrice(FeedInterface(tokenInfo.nativeOracle));
-            (uint256 price2, bool isError2) = fetchPrice(FeedInterface(tokenInfo.tokenOracle));
+            (uint256 price1, bool isError1) = fetchPrice(FeedInterface(tokenInfo.nativeOracle), priceUpdateThreshold);
+            (uint256 price2, bool isError2) = fetchPrice(FeedInterface(tokenInfo.tokenOracle), priceUpdateThreshold);
             isError = isError1 || isError2;
             if (isError) return (0, 0, 0, isError);
             tokenPrice = (price2 * (10 ** 18)) / price1;
             tokenOracleDecimals = 18;
         } else {
-            (tokenPrice, isError) = fetchPrice(FeedInterface(tokenInfo.tokenOracle));
+            (tokenPrice, isError) = fetchPrice(FeedInterface(tokenInfo.tokenOracle), priceUpdateThreshold);
             tokenOracleDecimals = FeedInterface(tokenInfo.tokenOracle).decimals();
         }
     }
@@ -72,15 +75,14 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
      * @param _oracle The Oracle contract to fetch the price from.
      * @return price The latest price fetched from the Oracle.
      */
-    function fetchPrice(FeedInterface _oracle) internal view returns (uint256 price, bool isError) {
+    function fetchPrice(FeedInterface _oracle, uint24 _priceUpdateThreshold) internal view returns (uint256 price, bool isError) {
         try _oracle.latestRoundData() returns (
             uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound
         ) {
             // validateRound
             if (answer <= 0) return (0, true);
             // 2 days old price is considered stale since the price is updated every 24 hours
-            if (updatedAt < block.timestamp - 60 * 60 * 24 * 2) return (0, true);
-            if (answeredInRound < roundId) return (0, true);
+            if (updatedAt < block.timestamp - _priceUpdateThreshold) return (0, true);
             price = uint256(answer);
             return (price, false);
         } catch Error(string memory reason) {

scripts/configs/prod/token-config-mumbai.json

@@ -26,7 +26,8 @@
         "address": "0xE03489D4E90b22c59c5e23d45DFd59Fc0dB8a025",
         "nativeOracleAddress": "0xd0D5e3DB44DE05E9F294BB0a3bEEaF030DE24Ada",
         "tokenOracleAddress": "0x9dd18534b8f456557d11B9DDB14dA89b2e52e308",
-        "derivedFeed": true
+        "derivedFeed": true,
+        "priceUpdateThreshold": 43200
       }
     ]
 }