fix: remediations as per discussion

livingrockrises · livingrockrises · commit 7d9fb24b2b41 · 2024-11-18T13:23:46.000+07:00
diff --git a/contracts/interfaces/IBiconomyTokenPaymaster.sol b/contracts/interfaces/IBiconomyTokenPaymaster.sol
@@ -18,7 +18,7 @@ interface IBiconomyTokenPaymaster {
     }
 
     event UpdatedUnaccountedGas(uint256 indexed oldValue, uint256 indexed newValue);
-    event UpdatedFixedPriceMarkup(uint256 indexed oldValue, uint256 indexed newValue);
+    event UpdatedFixedPriceMarkup(uint32 indexed oldValue, uint32 indexed newValue);
     event UpdatedVerifyingSigner(address indexed oldSigner, address indexed newSigner, address indexed actor);
     event UpdatedFeeCollector(address indexed oldFeeCollector, address indexed newFeeCollector, address indexed actor);
     event UpdatedPriceExpiryDuration(uint256 indexed oldValue, uint256 indexed newValue);
@@ -30,23 +30,27 @@ interface IBiconomyTokenPaymaster {
         address indexed token,
         uint256 nativeCharge,
         uint256 tokenCharge,
-        uint256 priceMarkup,
+        uint32 priceMarkup,
+        uint256 tokenPrice,
         bytes32 indexed userOpHash
     );
     event Received(address indexed sender, uint256 value);
     event TokensWithdrawn(address indexed token, address indexed to, uint256 indexed amount, address actor);
-    event UpdatedTokenDirectory(address indexed tokenAddress, IOracle indexed oracle, uint8 decimals);
+    event AddedToTokenDirectory(address indexed tokenAddress, IOracle indexed oracle, uint8 decimals);
+    event RemovedFromTokenDirectory(address indexed tokenAddress);
     event UpdatedNativeAssetOracle(IOracle indexed oldOracle, IOracle indexed newOracle);
+    event TokensSwappedAndRefilledEntryPoint(address indexed tokenAddress, uint256 indexed tokenAmount, uint256 indexed amountOut, address actor);
+    event SwappableTokensAdded(address[] indexed tokenAddresses);
 
     function setSigner(address newVerifyingSigner) external payable;
 
     function setUnaccountedGas(uint256 value) external payable;
 
-    function setPriceMarkup(uint256 newUnaccountedGas) external payable;
+    function setPriceMarkup(uint32 newPriceMarkup) external payable;
 
     function setPriceExpiryDuration(uint256 newPriceExpiryDuration) external payable;
 
     function setNativeAssetToUsdOracle(IOracle oracle) external payable;
 
-    function updateTokenDirectory(address tokenAddress, IOracle oracle) external payable;
+    function addToTokenDirectory(address tokenAddress, IOracle oracle) external payable;
 }
diff --git a/contracts/libraries/TokenPaymasterParserLib.sol b/contracts/libraries/TokenPaymasterParserLib.sol
@@ -31,17 +31,17 @@ library TokenPaymasterParserLib {
             uint48 validUntil,
             uint48 validAfter,
             address tokenAddress,
-            uint128 tokenPrice, // Review: why uint128 and not uint256. in independent mode it is uint256
+            uint256 tokenPrice, // Review: why uint128 and not uint256. in independent mode it is uint256
             uint32 externalPriceMarkup,
             bytes memory signature
         )
     {
         validUntil = uint48(bytes6(modeSpecificData[:6]));
         validAfter = uint48(bytes6(modeSpecificData[6:12]));
         tokenAddress = address(bytes20(modeSpecificData[12:32]));
-        tokenPrice = uint128(bytes16(modeSpecificData[32:48]));
-        externalPriceMarkup = uint32(bytes4(modeSpecificData[48:52]));
-        signature = modeSpecificData[52:];
+        tokenPrice = uint256(bytes32(modeSpecificData[32:64]));
+        externalPriceMarkup = uint32(bytes4(modeSpecificData[64:68]));
+        signature = modeSpecificData[68:];
     }
 
     function parseIndependentModeSpecificData(
diff --git a/contracts/token/BiconomyTokenPaymaster.sol b/contracts/token/BiconomyTokenPaymaster.sol
@@ -51,30 +51,25 @@ contract BiconomyTokenPaymaster is
     // State variables
     address public verifyingSigner; // entity used to provide external token price and markup
     uint256 public unaccountedGas;
-    uint256 public independentPriceMarkup; // price markup used for independent mode
+    uint32 public independentPriceMarkup; // price markup used for independent mode
     uint256 public priceExpiryDuration; // oracle price expiry duration
     IOracle public nativeAssetToUsdOracle; // ETH -> USD price oracle
     mapping(address => TokenInfo) public independentTokenDirectory; // mapping of token address => info for tokens
     // supported in // independent mode
 
-    // PAYMASTER_ID_OFFSET
-    // Note: Temp
     uint256 private constant _UNACCOUNTED_GAS_LIMIT = 200_000; // Limit for unaccounted gas cost
-    uint256 private constant _PRICE_DENOMINATOR = 1e6; // Denominator used when calculating cost with price markup
-    uint256 private constant _MAX_PRICE_MARKUP = 2e6; // 100% premium on price (2e6/PRICE_DENOMINATOR)
-
-    // Note: _priceExpiryDuration is common for all the feeds.
-    // Note: _independentPriceMarkup is common for all the independent tokens.
-    // Todo: add cases when uniswap is not available
-    // Note: swapTokenAndDeposit: we may not need to keep this onlyOwner
+    uint32 private constant _PRICE_DENOMINATOR = 1e6; // Denominator used when calculating cost with price markup
+    uint32 private constant _MAX_PRICE_MARKUP = 2e6; // 100% premium on price (2e6/PRICE_DENOMINATOR)
+    uint256 private immutable _NATIVE_TOKEN_DECIMALS;
 
     constructor(
         address owner,
         address verifyingSignerArg,
         IEntryPoint entryPoint,
         uint256 unaccountedGasArg,
-        uint256 independentPriceMarkupArg, // price markup used for independent mode
+        uint32 independentPriceMarkupArg, // price markup used for independent mode
         uint256 priceExpiryDurationArg,
+        uint256 nativeAssetDecimalsArg,
         IOracle nativeAssetToUsdOracleArg,
         ISwapRouter uniswapRouterArg,
         address wrappedNativeArg,
@@ -87,6 +82,7 @@ contract BiconomyTokenPaymaster is
         BasePaymaster(owner, entryPoint)
         Uniswapper(uniswapRouterArg, wrappedNativeArg, swappableTokens, swappableTokenPoolFeeTiers)
     {
+        _NATIVE_TOKEN_DECIMALS = nativeAssetDecimalsArg;
         if (_isContract(verifyingSignerArg)) {
             revert VerifyingSignerCanNotBeContract();
         }
@@ -107,6 +103,7 @@ contract BiconomyTokenPaymaster is
             // ETH -> USD will always have 8 decimals for Chainlink and TWAP
             revert InvalidOracleDecimals();
         }
+        require(block.timestamp >= priceExpiryDurationArg, "Price expiry duration cannot be in the past");
 
         // Set state variables
         assembly ("memory-safe") {
@@ -126,11 +123,6 @@ contract BiconomyTokenPaymaster is
             independentTokenDirectory[independentTokensArg[i]] =
                 TokenInfo(oraclesArg[i], 10 ** IERC20Metadata(independentTokensArg[i]).decimals());
         }
-        // Approve swappable tokens for max amount
-        uint256 length = swappableTokens.length;
-        for (uint256 i; i < length; i++) {
-            IERC20(swappableTokens[i]).approve(address(uniswapRouterArg), type(uint256).max);
-        }
     }
 
     /**
@@ -207,8 +199,8 @@ contract BiconomyTokenPaymaster is
      * @dev Set a new verifying signer address.
      * Can only be called by the owner of the contract.
      * @param newVerifyingSigner The new address to be set as the verifying signer.
-     * @notice If _newVerifyingSigner is set to zero address, it will revert with an error.
-     * After setting the new signer address, it will emit an event VerifyingSignerChanged.
+     * @notice If newVerifyingSigner is set to zero address, it will revert with an error.
+     * After setting the new signer address, it will emit an event UpdatedVerifyingSigner.
      */
     function setSigner(address newVerifyingSigner) external payable onlyOwner {
         if (_isContract(newVerifyingSigner)) revert VerifyingSignerCanNotBeContract();
@@ -243,24 +235,25 @@ contract BiconomyTokenPaymaster is
      * @param newIndependentPriceMarkup The new value to be set as the price markup
      * @notice only to be called by the owner of the contract.
      */
-    function setPriceMarkup(uint256 newIndependentPriceMarkup) external payable onlyOwner {
+    function setPriceMarkup(uint32 newIndependentPriceMarkup) external payable onlyOwner {
         if (newIndependentPriceMarkup > _MAX_PRICE_MARKUP || newIndependentPriceMarkup < _PRICE_DENOMINATOR) {
             // Not between 0% and 100% markup
             revert InvalidPriceMarkup();
         }
-        uint256 oldIndependentPriceMarkup = independentPriceMarkup;
+        uint32 oldIndependentPriceMarkup = independentPriceMarkup;
         assembly ("memory-safe") {
             sstore(independentPriceMarkup.slot, newIndependentPriceMarkup)
         }
         emit UpdatedFixedPriceMarkup(oldIndependentPriceMarkup, newIndependentPriceMarkup);
     }
 
     /**
-     * @dev Set a new priceMarkup value.
-     * @param newPriceExpiryDuration The new value to be set as the unaccounted gas value
+     * @dev Set a new price expiry duration.
+     * @param newPriceExpiryDuration The new value to be set as the price expiry duration
      * @notice only to be called by the owner of the contract.
      */
     function setPriceExpiryDuration(uint256 newPriceExpiryDuration) external payable onlyOwner {
+        require(block.timestamp >= newPriceExpiryDuration, "Price expiry duration cannot be in the past");
         uint256 oldPriceExpiryDuration = priceExpiryDuration;
         assembly ("memory-safe") {
             sstore(priceExpiryDuration.slot, newPriceExpiryDuration)
@@ -293,7 +286,7 @@ contract BiconomyTokenPaymaster is
      * @param oracle The oracle to use for the specified token
      * @notice only to be called by the owner of the contract.
      */
-    function updateTokenDirectory(address tokenAddress, IOracle oracle) external payable onlyOwner {
+    function addToTokenDirectory(address tokenAddress, IOracle oracle) external payable onlyOwner {
         if (oracle.decimals() != 8) {
             // Token -> USD will always have 8 decimals
             revert InvalidOracleDecimals();
@@ -302,7 +295,17 @@ contract BiconomyTokenPaymaster is
         uint8 decimals = IERC20Metadata(tokenAddress).decimals();
         independentTokenDirectory[tokenAddress] = TokenInfo(oracle, 10 ** decimals);
 
-        emit UpdatedTokenDirectory(tokenAddress, oracle, decimals);
+        emit AddedToTokenDirectory(tokenAddress, oracle, decimals);
+    }
+
+    /**
+     * @dev Remove a token from the independentTokenDirectory mapping.
+     * @param tokenAddress The token address to remove from directory
+     * @notice only to be called by the owner of the contract.
+     */
+    function removeFromTokenDirectory(address tokenAddress) external payable onlyOwner {
+        delete independentTokenDirectory[tokenAddress];
+        emit RemovedFromTokenDirectory(tokenAddress );
     }
 
     /**
@@ -326,6 +329,7 @@ contract BiconomyTokenPaymaster is
         for (uint256 i = 0; i < tokenAddresses.length; ++i) {
             _setTokenPool(tokenAddresses[i], poolFeeTiers[i]);
         }
+        emit SwappableTokensAdded(tokenAddresses);
     }
 
     /**
@@ -342,7 +346,7 @@ contract BiconomyTokenPaymaster is
     )
         external
         payable
-        onlyOwner
+        nonReentrant
     {
         // Swap tokens for WETH
         uint256 amountOut = _swapTokenToWeth(tokenAddress, tokenAmount, minEthAmountRecevied);
@@ -352,6 +356,7 @@ contract BiconomyTokenPaymaster is
             // Deposit ETH into EP
             entryPoint.depositTo{ value: amountOut }(address(this));
         }
+        emit TokensSwappedAndRefilledEntryPoint(tokenAddress, tokenAmount, amountOut, msg.sender);
     }
 
     /**
@@ -385,7 +390,7 @@ contract BiconomyTokenPaymaster is
         uint48 validUntil,
         uint48 validAfter,
         address tokenAddress,
-        uint128 tokenPrice,
+        uint256 tokenPrice,
         uint32 externalPriceMarkup
     )
         public
@@ -453,8 +458,7 @@ contract BiconomyTokenPaymaster is
                 uint48 validUntil,
                 uint48 validAfter,
                 address tokenAddress,
-                // Review if uint128 is enough
-                uint128 tokenPrice, // NotE: what backend should pass is token/native * 10^token decimals
+                uint256 tokenPrice, // NotE: what backend should pass is token/native * 10^token decimals
                 uint32 externalPriceMarkup,
                 bytes memory signature
             ) = modeSpecificData.parseExternalModeSpecificData();
@@ -485,15 +489,15 @@ contract BiconomyTokenPaymaster is
             {
                 uint256 maxFeePerGas = UserOperationLib.unpackMaxFeePerGas(userOp);
                 tokenAmount = ((maxCost + maxPenalty + (unaccountedGas * maxFeePerGas)) * externalPriceMarkup * tokenPrice)
-                    / (1e18 * _PRICE_DENOMINATOR);
+                    / (_NATIVE_TOKEN_DECIMALS * _PRICE_DENOMINATOR);
             }
 
             // Transfer full amount to this address. Unused amount will be refunded in postOP
             SafeTransferLib.safeTransferFrom(tokenAddress, userOp.sender, address(this), tokenAmount);
 
             // deduct max penalty from the token amount we pass to the postOp
             // so we don't refund it at postOp
-            context = abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice)/1e18), tokenPrice, externalPriceMarkup, userOpHash);
+            context = abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice)/_NATIVE_TOKEN_DECIMALS), tokenPrice, externalPriceMarkup, userOpHash);
             validationData = _packValidationData(false, validUntil, validAfter);
         } else if (mode == PaymasterMode.INDEPENDENT) {
             // Use only oracles for the token specified in modeSpecificData
@@ -504,21 +508,24 @@ contract BiconomyTokenPaymaster is
             // Get address for token used to pay
             address tokenAddress = modeSpecificData.parseIndependentModeSpecificData();
             uint256 tokenPrice = _getPrice(tokenAddress);
+            if(tokenPrice == 0) {
+                revert TokenNotSupported();
+            }
             uint256 tokenAmount;
 
             // TODO: Account for penalties here
             {
                 // Calculate token amount to precharge
                 uint256 maxFeePerGas = UserOperationLib.unpackMaxFeePerGas(userOp);
                 tokenAmount = ((maxCost + maxPenalty + (unaccountedGas * maxFeePerGas)) * independentPriceMarkup * tokenPrice)
-                    / (1e18 * _PRICE_DENOMINATOR);
+                    / (_NATIVE_TOKEN_DECIMALS * _PRICE_DENOMINATOR);
             }
 
             // Transfer full amount to this address. Unused amount will be refunded in postOP
             SafeTransferLib.safeTransferFrom(tokenAddress, userOp.sender, address(this), tokenAmount);
 
             context =
-                abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice)/1e18), tokenPrice, independentPriceMarkup, userOpHash);
+                abi.encode(userOp.sender, tokenAddress, tokenAmount-((maxPenalty*tokenPrice)/_NATIVE_TOKEN_DECIMALS), tokenPrice, independentPriceMarkup, userOpHash);
             validationData = 0; // Validation success and price is valid indefinetly
         }
     }
@@ -545,14 +552,14 @@ contract BiconomyTokenPaymaster is
             address tokenAddress,
             uint256 prechargedAmount,
             uint192 tokenPrice,
-            uint256 appliedPriceMarkup,
+            uint32 appliedPriceMarkup,
             bytes32 userOpHash
-        ) = abi.decode(context, (address, address, uint256, uint192, uint256, bytes32));
+        ) = abi.decode(context, (address, address, uint256, uint192, uint32, bytes32));
 
         // Calculate the actual cost in tokens based on the actual gas cost and the token price
         uint256 actualTokenAmount = (
             (actualGasCost + (unaccountedGas * actualUserOpFeePerGas)) * appliedPriceMarkup * tokenPrice
-        ) / (1e18 * _PRICE_DENOMINATOR);
+        ) / (_NATIVE_TOKEN_DECIMALS * _PRICE_DENOMINATOR);
         if (prechargedAmount > actualTokenAmount) {
             // If the user was overcharged, refund the excess tokens
             uint256 refundAmount = prechargedAmount - actualTokenAmount;
@@ -562,7 +569,7 @@ contract BiconomyTokenPaymaster is
 
         // Todo: Review events and what we need to emit.
         emit PaidGasInTokens(
-            userOpSender, tokenAddress, actualGasCost, actualTokenAmount, appliedPriceMarkup, userOpHash
+            userOpSender, tokenAddress, actualGasCost, actualTokenAmount, appliedPriceMarkup, tokenPrice, userOpHash
         );
     }
 
@@ -578,8 +585,8 @@ contract BiconomyTokenPaymaster is
         }
 
         // Calculate price by using token and native oracle
-        uint192 tokenPrice = _fetchPrice(tokenInfo.oracle);
-        uint192 nativeAssetPrice = _fetchPrice(nativeAssetToUsdOracle);
+        uint256 tokenPrice = _fetchPrice(tokenInfo.oracle);
+        uint256 nativeAssetPrice = _fetchPrice(nativeAssetToUsdOracle);
 
         // Adjust to token  decimals
         price = (nativeAssetPrice * tokenInfo.decimals) / tokenPrice;
@@ -590,15 +597,15 @@ contract BiconomyTokenPaymaster is
     /// @param oracle The oracle contract to fetch the price from.
     /// @return price The latest price fetched from the oracle.
     /// Note: We could do this using oracle aggregator, so we can also use Pyth. or Twap based oracle and just not chainlink.
-    function _fetchPrice(IOracle oracle) internal view returns (uint192 price) {
+    function _fetchPrice(IOracle oracle) internal view returns (uint256 price) {
         (, int256 answer,, uint256 updatedAt,) = oracle.latestRoundData();
         if (answer <= 0) {
             revert OraclePriceNotPositive();
         }
         if (updatedAt < block.timestamp - priceExpiryDuration) {
             revert OraclePriceExpired();
         }
-        price = uint192(int192(answer));
+        price = uint256(answer);
     }
 
     function _withdrawERC20(IERC20 token, address target, uint256 amount) private {
diff --git a/contracts/token/swaps/Uniswapper.sol b/contracts/token/swaps/Uniswapper.sol
@@ -12,8 +12,6 @@ import "@uniswap/v3-periphery/contracts/interfaces/IPeripheryPayments.sol";
  * @notice Based on Infinitism's Uniswap Helper contract
  */
 abstract contract Uniswapper {
-    uint256 private constant _SWAP_PRICE_DENOMINATOR = 1e26;
-
     /// @notice The Uniswap V3 SwapRouter contract
     ISwapRouter public immutable uniswapRouter;
 
diff --git a/package.json b/package.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/bcnmy"
   },
   "dependencies": {
-    "@openzeppelin/contracts": "5.0.2",
+    "@openzeppelin/contracts": "5.1.0",
     "@rhinestone/modulekit": "^0.4.10",
     "@uniswap/v3-core": "https://github.com/Uniswap/v3-core#0.8",
     "@uniswap/v3-periphery": "https://github.com/Uniswap/v3-periphery#0.8",
diff --git a/test/base/TestBase.sol b/test/base/TestBase.sol
@@ -61,7 +61,7 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors {
         uint48 validUntil;
         uint48 validAfter;
         address tokenAddress;
-        uint128 tokenPrice;
+        uint256 tokenPrice;
         uint32 externalPriceMarkup;
     }
 
diff --git a/test/unit/concrete/TestTokenPaymaster.Base.t.sol b/test/unit/concrete/TestTokenPaymaster.Base.t.sol
@@ -42,6 +42,7 @@ contract TestTokenPaymasterBase is TestBase {
             50000, // unaccounted gas
             1e6, // price markup (for independent mode)
             1 days, // price expiry duration
+            1e18, // native token decimals
             nativeOracle,
             swapRouter,
             WRAPPED_NATIVE_ADDRESS,
@@ -61,6 +62,7 @@ contract TestTokenPaymasterBase is TestBase {
             50000, // unaccounted gas
             1e6, // price markup
             1 days, // price expiry duration
+            1e18, // native token decimals
             nativeOracle,
             swapRouter,
             WRAPPED_NATIVE_ADDRESS,
@@ -115,7 +117,7 @@ contract TestTokenPaymasterBase is TestBase {
         emit IBiconomyTokenPaymaster.TokensRefunded(address(ALICE_ACCOUNT), address(usdc), 0, bytes32(0));
 
         vm.expectEmit(true, true, false, false, address(tokenPaymaster));
-        emit IBiconomyTokenPaymaster.PaidGasInTokens(address(ALICE_ACCOUNT), address(usdc), 0, 0, 1e6, bytes32(0));
+        emit IBiconomyTokenPaymaster.PaidGasInTokens(address(ALICE_ACCOUNT), address(usdc), 0, 0, 1e6, 0, bytes32(0));
 
         startPrank(BUNDLER.addr);
         ENTRYPOINT.handleOps(ops, payable(BUNDLER.addr));
diff --git a/test/unit/concrete/TestTokenPaymaster.t.sol b/test/unit/concrete/TestTokenPaymaster.t.sol
diff --git a/test/unit/concrete/TestTokenPaymasterParserLib.t.sol b/test/unit/concrete/TestTokenPaymasterParserLib.t.sol

Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ abstract contract TestBase is CheatCodes, TestHelper, BaseEventsAndErrors {`
`61`	`61`	`uint48 validUntil;`
`62`	`62`	`uint48 validAfter;`
`63`	`63`	`address tokenAddress;`
`64`		`- uint128 tokenPrice;`
	`64`	`+ uint256 tokenPrice;`
`65`	`65`	`uint32 externalPriceMarkup;`
`66`	`66`	`}`
`67`	`67`