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/libs/MathLib.sol

@@ -4,21 +4,21 @@ pragma solidity ^0.8.23;
 library MathLib {
     function minuint256(uint256 a, uint256 b) internal pure returns (uint256 result) {
         assembly {
-            result := xor(b, mul(xor(b, a), gt(a, b)))
-        }
+        result := xor(b, mul(xor(b, a), gt(b, a)))
     }
+}
 
     function maxuint256(uint256 a, uint256 b) internal pure returns (uint256 result) {
         assembly {
             result := xor(a, mul(xor(a, b), gt(b, a)))
         }
     }
 
-    function minuint32(uint32 a, uint32 b) internal pure returns (uint32 result) {
+   function minuint32(uint32 a, uint32 b) internal pure returns (uint32 result) {
         assembly {
-            result := xor(b, mul(xor(b, a), gt(a, b)))
-        }
+            result := xor(b, mul(xor(b, a), gt(b, a)))
     }
+}
 
     function maxuint32(uint32 a, uint32 b) internal pure returns (uint32 result) {
         assembly {

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";
@@ -79,6 +80,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.
@@ -180,11 +185,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;
             }
@@ -198,7 +204,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;
@@ -285,71 +292,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
@@ -362,6 +304,8 @@ contract BiconomyTokenPaymaster is
         ExchangeRateSource priceSource;
         uint128 exchangeRate;
         uint32 priceMarkup;
+        uint256 maxFeePerGas;
+        uint256 maxPriorityFeePerGas;
         bytes32 userOpHash;
         assembly ("memory-safe") {
             let offset := context.offset
@@ -381,6 +325,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)
         }
 
@@ -391,11 +341,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);
         }
 
@@ -425,12 +380,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;
+        uint24 priceUpdateThreshold;
         address tokenOracle;
         address nativeOracle;
         bool isDerivedFeed;
     }
 
+    error MismatchInBaseAndQuoteDecimals();
+    error InvalidPriceFromRound();
+    error LatestRoundIncomplete();
+    error PriceFeedStale();
+    error OracleAddressCannotBeZero();
+
     function getTokenValueOfOneNativeToken(address _token) external view returns (uint128 exchangeRate);
 }

contracts/token/oracles/OracleAggregator.sol

@@ -2,6 +2,7 @@
 pragma solidity ^0.8.23;
 
 import "@openzeppelin/contracts/access/Ownable.sol";
+import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 import "./IOracleAggregator.sol";
 import "./FeedInterface.sol";
 
@@ -14,10 +15,10 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
 
     function setTokenOracle(
         address token,
-        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();
@@ -27,8 +28,9 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
         if (decimals1 != decimals2) revert MismatchInBaseAndQuoteDecimals();
         tokensInfo[token].tokenOracle = tokenOracle;
         tokensInfo[token].nativeOracle = nativeOracle;
-        tokensInfo[token].tokenDecimals = tokenDecimals;
+        tokensInfo[token].tokenDecimals = ERC20(token).decimals();
         tokensInfo[token].isDerivedFeed = isDerivedFeed;
+        tokensInfo[token].priceUpdateThreshold = priceUpdateThreshold;
     }
 
     /**
@@ -50,18 +52,19 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
         view
         returns (uint256 tokenPrice, uint8 tokenOracleDecimals, uint8 tokenDecimals, bool isError)
     {
-        TokenInfo storage tokenInfo = tokensInfo[token];
+        TokenInfo memory 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();
         }
     }
@@ -70,17 +73,18 @@ abstract contract OracleAggregator is Ownable, IOracleAggregator {
      * @dev This function is used to get the latest price from the tokenOracle or nativeOracle.
      * @notice Fetches the latest price from the given Oracle.
      * @param _oracle The Oracle contract to fetch the price from.
+     * @param _priceUpdateThreshold The time after which the price is considered stale.
      * @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);
+            // price older than set _priceUpdateThreshold is considered stale
+            // _priceUpdateThreshold for oracle feed is usually heartbeat interval + block time + buffer
+            if (updatedAt < block.timestamp - _priceUpdateThreshold) return (0, true);
             price = uint256(answer);
             return (price, false);
         } catch Error(string memory reason) {