core-review/respond to PR comments

bcnmy · May 10, 2023 · 6adddfb · 6adddfb
1 parent 713808a
commit 6adddfb
Show file tree

Hide file tree

Showing 6 changed files with 115 additions and 60 deletions.
diff --git a/contracts/token/BiconomyTokenPaymaster.sol b/contracts/token/BiconomyTokenPaymaster.sol
@@ -24,12 +24,15 @@ import {TokenPaymasterErrors} from "../common/Errors.sol";
 
 // Biconomy Token Paymaster
 /**
- * A token-based paymaster that accepts token deposits for supported ERC20 tokens.
- * It is an extension of VerifyingPaymaster which trusts external signer to authorize the transaction, but also with an ability to withdraw tokens. 
+ * A token-based paymaster that allows user to pay gas fee in ERC20 tokens. The paymaster owner chooses which tokens to accept.
+ * The payment manager (usually the owner) first deposits native gas into the EntryPoint. Then, for each transaction, it takes the gas fee from the user's ERC20 token balance. The manager must convert these collected tokens back to native gas and deposit it into the EntryPoint to keep the system running.
+ * It is an extension of VerifyingPaymaster which trusts external signer to authorize the transaction, but also with an ability to withdraw tokens.
+ * 
+ * The validatePaymasterUserOp function does not interact with external contracts but uses an externally provided exchange rate.
+ * Based on the exchangeRate and requiredPrefund amount, the validation method checks if the user's account has enough token balance. This is done by only looking at the referenced storage.
+ * All Withdrawn tokens are sent to a dynamic fee receiver address.
+ * 
  * Optionally a safe guard deposit may be used in future versions.
- * validatePaymasterUserOp doesn't call any external contracts but currently relies on exchangeRate passed externally.
- * based on the exchangeRate and requiredPrefund validation stage ensure the account has enough token allowance and balance, hence only checking referenced state.
- * All withdrawn tokens will be transferred to dynamic fee receiver address.
  */
 contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymasterErrors {
 
@@ -51,6 +54,8 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
     // receiver of withdrawn fee tokens
     address public feeReceiver;
 
+    // paymasterAndData: concat of [paymasterAddress(address), priceSource(enum 1 byte), abi.encode(validUntil, validAfter, feeToken, exchangeRate, fee): makes up 32*5 bytes, signature]
+    // PND offset is used to indicate offsets to decode, used along with Signature offset
     uint256 private constant VALID_PND_OFFSET = 21;
 
     uint256 private constant SIGNATURE_OFFSET = 181;
@@ -59,37 +64,60 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
     // notice: Since it's always verified by the signing service, below gated mapping state could be avoided.
     mapping(address => bool) private supportedTokens;
 
+    // Owned contract that manages chainlink price feeds (token / eth formaat) and helper to give exchange rate (inverse price)
     IOracleAggregator public oracleAggregator;
 
+    /**
+     * Designed to enable the community to track change in storage variable UNACCOUNTED_COST which is used
+     * to maintain gas execution cost which can't be calculated within contract*/
     event EPGasOverheadChanged(
-        uint256 indexed _oldValue,
-        uint256 indexed _newValue
+        uint256 indexed _oldOverheadCost,
+        uint256 indexed _newOverheadCost,
+        address indexed _actor
     );
 
+    /**
+     * Designed to enable the community to track change in storage variable verifyingSigner which is used
+     * to authorize any operation for this paymaster (validation stage) and provides signature*/
     event VerifyingSignerChanged(
         address indexed _oldSigner,
         address indexed _newSigner,
         address indexed _actor
     );
-
+
+    /**
+     * Designed to enable the community to track change in storage variable oracleAggregator which is a contract
+     * used to maintain price feeds for exchangeRate supported tokens*/
     event OracleAggregatorChanged(
         address indexed _oldoracleAggregator,
         address indexed _neworacleAggregator,
         address indexed _actor
     );
 
+
+    /**
+     * Designed to enable the community to track change in storage variable feeReceiver which is an address (self or other SCW/EOA)
+     * responsible for collecting all the tokens being withdrawn as fees*/
     event FeeReceiverChanged(
         address indexed _oldfeeReceiver,
         address indexed _newfeeReceiver,
         address indexed _actor
     );
 
-    event NewFeeTokenSupported(
+    /**
+     * Designed to enable the community to track change in supported ERC20 tokens. Note that a token supported earlier
+     * can be denied*/
+    event TokenSupportedOrRevoked(
         address indexed _token,
+        bool indexed _allowed,
         address indexed _actor
     );
 
-    event TokenPaymasterOperation(address indexed sender, address indexed token, uint256 charge, uint256 premium);
+
+    /**
+     * 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*/
+    event TokenPaymasterOperation(address indexed sender, address indexed token, uint256 indexed charge, uint256 premium);
 
     constructor(
         address _owner,
@@ -102,14 +130,12 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
         if(_owner == address(0)) revert OwnerCannotBeZero();
         if (address(_entryPoint) == address(0)) revert EntryPointCannotBeZero();
         if(address(_oracleAggregator) == address(0)) revert OracleAggregatorCannotBeZero();
-        if (_verifyingSigner == address(0))
-            revert VerifyingSignerCannotBeZero();
-         _transferOwnership(_owner);
+        if (_verifyingSigner == address(0)) revert VerifyingSignerCannotBeZero();
         assembly {
             sstore(verifyingSigner.slot, _verifyingSigner)
+            sstore(oracleAggregator.slot, _oracleAggregator)
+            sstore(feeReceiver.slot, address())  // initialize with self (could also be _owner)
         }
-        oracleAggregator = _oracleAggregator;
-        feeReceiver = address(this); // initialize with self (could also be _owner)
     }
 
     /**
@@ -129,8 +155,13 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
         emit VerifyingSignerChanged(oldSigner, _newVerifyingSigner, msg.sender);
     }
 
-    // notice payable in setVerifyingSigner
-    // todo review payable in onlyOwner methods from GO POV
+    /**
+     * @dev Set a new oracle aggregator.
+     * Can only be called by the owner of the contract.
+     * @param _newOracleAggregator The new address to be set as the address of oracle aggregator contract.
+     * @notice If _newOracleAggregator is set to zero address, it will revert with an error.
+     * After setting the new address, it will emit an event OracleAggregatorChanged.
+     */
     function setOracleAggregator(address _newOracleAggregator) external payable onlyOwner {
         if (_newOracleAggregator == address(0))
             revert OracleAggregatorCannotBeZero();
@@ -141,6 +172,13 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
         emit OracleAggregatorChanged(oldOA, _newOracleAggregator, msg.sender);
     }
 
+    /**
+     * @dev Set a new fee receiver.
+     * Can only be called by the owner of the contract.
+     * @param _newFeeReceiver The new address to be set as the address of new fee receiver.
+     * @notice If _newFeeReceiver is set to zero address, it will revert with an error.
+     * After setting the new address, it will emit an event FeeReceiverChanged.
+     */
     function setFeeReceiver(address _newFeeReceiver) external payable onlyOwner {
          if (_newFeeReceiver == address(0))
             revert FeeReceiverCannotBeZero();
@@ -152,20 +190,37 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
 
     }
 
-    function setTokenAllowed(address _token) external payable onlyOwner {
+    /**
+     * @dev Allow a new token or revoke previously enabled ERC20 token.
+     * Can only be called by the owner of the contract.
+     * @param _token ERC20 address
+     * @param _allowed if new token is being allowed it will be true, for revoking already supported token it will be false
+     * @notice If _token is set to zero address, it will revert with an error.
+     * After allow/deny of the token, it will emit an event TokenSupportedOrRevoked.
+     */
+    function setTokenAllowed(address _token, bool _allowed) external payable onlyOwner {
         require(_token != address(0), "Token address cannot be zero");
-        supportedTokens[_token] = true;
-        emit NewFeeTokenSupported(_token, msg.sender);
+        supportedTokens[_token] = _allowed;
+        emit TokenSupportedOrRevoked(_token, _allowed, msg.sender);
     }
 
-    function setUnaccountedEPGasOverhead(uint256 value) external payable onlyOwner {
+    /**
+     * @dev Set a new overhead for unaccounted cost
+     * Can only be called by the owner of the contract.
+     * @param _newOverheadCost The new value to be set as the gas cost overhead.
+     * @notice If _newOverheadCost is set to very high value, it will revert with an error.
+     * After setting the new value, it will emit an event EPGasOverheadChanged.
+     */
+    function setUnaccountedEPGasOverhead(uint256 _newOverheadCost) external payable onlyOwner {
+        require(_newOverheadCost < 200000, "_newOverheadCost can not be unrealistic");
         uint256 oldValue = UNACCOUNTED_COST;
-        UNACCOUNTED_COST = value;
-        emit EPGasOverheadChanged(oldValue, value);
+        UNACCOUNTED_COST = _newOverheadCost;
+        emit EPGasOverheadChanged(oldValue, _newOverheadCost, msg.sender);
     }
 
     /**
-     * add a deposit for this paymaster, used for paying for transaction fees
+     * Add a deposit in native currency for this paymaster, used for paying for transaction fees. 
+     * This is ideally done by the entity who is managing the received ERC20 gas tokens.
      */
     function deposit() public payable virtual override nonReentrant {
         IEntryPoint(entryPoint).depositTo{value: msg.value}(address(this));
@@ -186,14 +241,11 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
 
     /**
      * @dev Returns true if this contract supports the given fee token address.
+     * @param _token ERC20 token address 
      */
     function isSupportedToken(
         address _token
-    ) external view virtual returns (bool) {
-        return _isSupportedToken(_token);
-    }
-
-    function _isSupportedToken(address _token) private view returns (bool) {
+    ) public view virtual returns (bool) {
         return supportedTokens[_token];
     }
 
@@ -216,7 +268,7 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
      * @param target address to send to
      * @param amount amount to withdraw
      */
-    function withdrawTokensTo(IERC20 token, address target, uint256 amount) public nonReentrant {
+    function withdrawERC20To(IERC20 token, address target, uint256 amount) public nonReentrant {
         require(owner() == msg.sender, "only owner can withdraw tokens"); // add revert code
         token.safeTransfer(target, amount);
     }
@@ -265,7 +317,8 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
 
     /**
      * @dev Verify that an external signer signed the paymaster data of a user operation.
-     * The paymaster data is expected to be the paymaster and a signature over the entire request parameters.
+     * 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, fee), 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.
@@ -313,17 +366,19 @@ contract BiconomyTokenPaymaster is BasePaymaster, ReentrancyGuard, TokenPaymaste
 
         address account = userOp.getSender();
 
-        require(_isSupportedToken(feeToken), "TokenPaymaster: token is not supported as fee token") ;
+        require(isSupportedToken(feeToken), "TokenPaymaster: token is not supported as fee token") ;
 
         uint256 costOfPost = userOp.maxFeePerGas * UNACCOUNTED_COST; // unaccountedEPGasOverhead
 
         // This model assumes irrespective of priceSource exchangeRate is always sent from outside
         // for below checks you would either need maxCost or some exchangeRate
+
+        // can add some checks here on calculated value, fee cap, exchange rate deviation/cap etc
         uint256 tokenRequiredPreFund = ((requiredPreFund + costOfPost) * exchangeRate) / 10 ** 18;
 
         require(
             IERC20(feeToken).balanceOf(account) >= (tokenRequiredPreFund + fee),
-            "Token Paymaster: not enough balance"
+            "Token Paymaster: account does not have enough token balance"
         );
 
         context = abi.encode(account, feeToken, priceSource, exchangeRate, fee, userOp.maxFeePerGas,

diff --git a/scripts/deploy-token-paymaster-mumbai.ts b/scripts/deploy-token-paymaster-mumbai.ts
@@ -49,7 +49,7 @@ tx = await oracleAggregator.setTokenOracle(
 receipt = await tx.wait();
 console.log("Oracle set for USDC");
 
-tx = await tokenPaymaster.setTokenAllowed(usdcAddress);
+tx = await tokenPaymaster.setTokenAllowed(usdcAddress, true);
 receipt = await tx.wait();
 console.log("Token is marked allowed");
 

diff --git a/scripts/deploy-token-paymaster-polygon-mainnet.ts b/scripts/deploy-token-paymaster-polygon-mainnet.ts
@@ -47,7 +47,7 @@ tx = await oracleAggregator.setTokenOracle(
 receipt = await tx.wait();
 console.log("Oracle set for USDC");
 
-tx = await tokenPaymaster.setTokenAllowed(usdcAddress);
+tx = await tokenPaymaster.setTokenAllowed(usdcAddress, true);
 receipt = await tx.wait();
 console.log("Token is marked allowed");
 

diff --git a/test/token-paymaster/biconomy-token-paymaster-specs.ts b/test/token-paymaster/biconomy-token-paymaster-specs.ts
@@ -153,7 +153,7 @@ describe("Biconomy Token Paymaster", function () {
       oracleAggregator.address
     );
 
-    await sampleTokenPaymaster.setTokenAllowed(token.address);
+    await sampleTokenPaymaster.setTokenAllowed(token.address, true);
 
     smartWalletImp = await new BiconomyAccountImplementation__factory(deployer).deploy(
       entryPoint.address

diff --git a/test/token-paymaster/btpm-undeployed-wallet.ts b/test/token-paymaster/btpm-undeployed-wallet.ts
@@ -153,7 +153,7 @@ describe("Biconomy Token Paymaster", function () {
       oracleAggregator.address
     );
 
-    await sampleTokenPaymaster.setTokenAllowed(token.address);
+    await sampleTokenPaymaster.setTokenAllowed(token.address, true);
 
     smartWalletImp = await new BiconomyAccountImplementation__factory(deployer).deploy(
       entryPoint.address

diff --git a/yarn.lock b/yarn.lock
@@ -2,9 +2,9 @@
 # yarn lockfile v1
 
 
-"@0xsequence/create3@https://github.com/0xsequence/create3":
+"@0xsequence/create3@git+https://github.com/0xsequence/create3.git":
   version "3.0.0"
-  resolved "https://github.com/0xsequence/create3#acc4703a21ec1d71dc2a99db088c4b1f467530fd"
+  resolved "git+https://github.com/0xsequence/create3.git#acc4703a21ec1d71dc2a99db088c4b1f467530fd"
   dependencies:
     csv-writer "^1.6.0"
 
@@ -1186,9 +1186,9 @@
   integrity sha512-yOlFc+7UtL/89t2ZhjPvvB/DeAr3r+Dq58IgzsFkOAvVC6NMJXmCGjbptdXdR9qsX7pKcTL+s87FtYREi2dEEQ==
 
 "@typechain/ethers-v5@^10.2.0":
-  version "10.2.0"
-  resolved "https://registry.yarnpkg.com/@typechain/ethers-v5/-/ethers-v5-10.2.0.tgz#68f5963efb5214cb2d881477228e4b5b315473e1"
-  integrity sha512-ikaq0N/w9fABM+G01OFmU3U3dNnyRwEahkdvi9mqy1a3XwKiPZaF/lu54OcNaEWnpvEYyhhS0N7buCtLQqC92w==
+  version "10.2.1"
+  resolved "https://registry.yarnpkg.com/@typechain/ethers-v5/-/ethers-v5-10.2.1.tgz#50241e6957683281ecfa03fb5a6724d8a3ce2391"
+  integrity sha512-n3tQmCZjRE6IU4h6lqUGiQ1j866n5MTCBJreNEHHVWXa2u9GJTaeYyU1/k+1qLutkyw+sS6VAN+AbeiTqsxd/A==
   dependencies:
     lodash "^4.17.15"
     ts-essentials "^7.0.1"
@@ -1201,9 +1201,9 @@
     fs-extra "^9.1.0"
 
 "@typechain/hardhat@^6.1.5":
-  version "6.1.5"
-  resolved "https://registry.yarnpkg.com/@typechain/hardhat/-/hardhat-6.1.5.tgz#caad58a1d3e9cd88061a584eb4f4fa763d5dcad1"
-  integrity sha512-lg7LW4qDZpxFMknp3Xool61Fg6Lays8F8TXdFGBG+MxyYcYU5795P1U2XdStuzGq9S2Dzdgh+1jGww9wvZ6r4Q==
+  version "6.1.6"
+  resolved "https://registry.yarnpkg.com/@typechain/hardhat/-/hardhat-6.1.6.tgz#1a749eb35e5054c80df531cf440819cb347c62ea"
+  integrity sha512-BiVnegSs+ZHVymyidtK472syodx1sXYlYJJixZfRstHVGYTi8V1O7QG4nsjyb0PC/LORcq7sfBUcHto1y6UgJA==
   dependencies:
     fs-extra "^9.1.0"
 
@@ -1312,9 +1312,9 @@
   integrity sha512-iiUgKzV9AuaEkZqkOLDIvlQiL6ltuZd9tGcW3gwpnX8JbuiuhFlEGmmFXEXkN50Cvq7Os88IY2v0dkDqXYWVgA==
 
 "@types/node@*":
-  version "20.1.0"
-  resolved "https://registry.yarnpkg.com/@types/node/-/node-20.1.0.tgz#258805edc37c327cf706e64c6957f241ca4c4c20"
-  integrity sha512-O+z53uwx64xY7D6roOi4+jApDGFg0qn6WHcxe5QeqjMaTezBO/mxdfFXIVAVVyNWKx84OmPB3L8kbVYOTeN34A==
+  version "20.1.2"
+  resolved "https://registry.yarnpkg.com/@types/node/-/node-20.1.2.tgz#8fd63447e3f99aba6c3168fd2ec4580d5b97886f"
+  integrity sha512-CTO/wa8x+rZU626cL2BlbCDzydgnFNgc19h4YvizpTO88MFQxab8wqisxaofQJ/9bLGugRdWIuX/TbIs6VVF6g==
 
 "@types/node@^10.0.3":
   version "10.17.60"
@@ -1327,9 +1327,9 @@
   integrity sha512-J8xLz7q2OFulZ2cyGTLE1TbbZcjpno7FaN6zdJNrgAdrJ+DZzh/uFR6YrTb4C+nXakvud8Q4+rbhoIWlYQbUFQ==
 
 "@types/node@^18.16.0":
-  version "18.16.5"
-  resolved "https://registry.yarnpkg.com/@types/node/-/node-18.16.5.tgz#bf64e42719dc2e74da24709a2e1c0b50a966120a"
-  integrity sha512-seOA34WMo9KB+UA78qaJoCO20RJzZGVXQ5Sh6FWu0g/hfT44nKXnej3/tCQl7FL97idFpBhisLYCTB50S0EirA==
+  version "18.16.7"
+  resolved "https://registry.yarnpkg.com/@types/node/-/node-18.16.7.tgz#86d0ba2541f808cb78d4dc5d3e40242a349d6db8"
+  integrity sha512-MFg7ua/bRtnA1hYE3pVyWxGd/r7aMqjNOdHvlSsXV3n8iaeGKkOaPzpJh6/ovf4bEXWcojkeMJpTsq3mzXW4IQ==
 
 "@types/node@^8.0.0":
   version "8.10.66"
@@ -2084,9 +2084,9 @@ camelcase@^6.0.0:
   integrity sha512-Gmy6FhYlCY7uOElZUSbxo2UCDH8owEk996gkbrpsgGtrJLM3J7jGxl9Ic7Qwwj4ivOE5AWZWRMecDdF7hqGjFA==
 
 caniuse-lite@^1.0.30001449:
-  version "1.0.30001485"
-  resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001485.tgz#026bb7319f1e483391872dc303a973d4f513f619"
-  integrity sha512-8aUpZ7sjhlOyiNsg+pgcrTTPUXKh+rg544QYHSvQErljVEKJzvkYkCR/hUFeeVoEfTToUtY9cUKNRC7+c45YkA==
+  version "1.0.30001486"
+  resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001486.tgz#56a08885228edf62cbe1ac8980f2b5dae159997e"
+  integrity sha512-uv7/gXuHi10Whlj0pp5q/tsK/32J2QSqVRKQhs2j8VsDCjgyruAh/eEXHF822VqO9yT6iZKw3nRwZRSPBE9OQg==
 
 case@^1.6.3:
   version "1.6.3"
@@ -2688,9 +2688,9 @@ [email protected]:
   integrity sha512-WMwm9LhRUo+WUaRN+vRuETqG89IgZphVSNkdFgeb6sS/E4OrDIN7t48CAewSHXc6C8lefD8KKfr5vY61brQlow==
 
 electron-to-chromium@^1.4.284:
-  version "1.4.385"
-  resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.4.385.tgz#1afd8d6280d510145148777b899ff481c65531ff"
-  integrity sha512-L9zlje9bIw0h+CwPQumiuVlfMcV4boxRjFIWDcLfFqTZNbkwOExBzfmswytHawObQX4OUhtNv8gIiB21kOurIg==
+  version "1.4.388"
+  resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.4.388.tgz#ec0d1be823d5b14da56d91ec5c57e84b4624ea45"
+  integrity sha512-xZ0y4zjWZgp65okzwwt00f2rYibkFPHUv9qBz+Vzn8cB9UXIo9Zc6Dw81LJYhhNt0G/vR1OJEfStZ49NKl0YxQ==
 
 [email protected], elliptic@^6.4.0, elliptic@^6.5.2, elliptic@^6.5.4:
   version "6.5.4"
@@ -5404,9 +5404,9 @@ [email protected]:
     whatwg-url "^5.0.0"
 
 node-fetch@^2.6.0, node-fetch@^2.6.1, node-fetch@^2.6.7:
-  version "2.6.9"
-  resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.9.tgz#7c7f744b5cc6eb5fd404e0c7a9fec630a55657e6"
-  integrity sha512-DJm/CJkZkRjKKj4Zi4BsKVZh3ValV5IR5s7LVZnW+6YMh0W1BfNA8XSs6DLMGYlId5F3KnA70uu2qepcR08Qqg==
+  version "2.6.11"
+  resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.11.tgz#cde7fc71deef3131ef80a738919f999e6edfff25"
+  integrity sha512-4I6pdBY1EthSqDmJkiNk3JIT8cswwR9nfeW/cPdUagJYEQG7R95WRH74wpz7ma8Gh/9dI9FP+OU+0E4FvtA55w==
   dependencies:
     whatwg-url "^5.0.0"