Merge pull request #126 from davidinsuomi/develop

Update Paymaster
SoulWallet · Nov 19, 2023 · f456e77 · f456e77
2 parents 5d0822a + 28d9894
commit f456e77
Show file tree

Hide file tree

Showing 3 changed files with 117 additions and 14 deletions.
diff --git a/contracts/paymaster/ERC20Paymaster.sol b/contracts/paymaster/ERC20Paymaster.sol
@@ -124,42 +124,57 @@ contract ERC20Paymaster is BasePaymaster {
         return (abi.encode(sender, token, costOfPost, exchangeRate), 0);
     }
 
+    /*
+    * @notice This function is currently in the testing phase.
+    * @dev The Paymaster is potentially vulnerable to attacks, which poses a risk of reputation loss.
+    * The approval check in this context does not guarantee that the Paymaster will successfully receive the corresponding tokens via transferFrom in subsequent _postOp operations.
+    */
     function _validateConstructor(UserOperation calldata userOp, address token, uint256 tokenRequiredPreFund)
         internal
         view
     {
         address factory = address(bytes20(userOp.initCode));
         require(factory == WALLET_FACTORY, "Paymaster: unknown wallet factory");
         require(
-            bytes4(userOp.callData) == bytes4(0x18dfb3c7 /* 0x18dfb3c7 executeBatch(address[],bytes[]) */ ),
+            /*
+            * 0x18dfb3c7 executeBatch(address[],bytes[])
+            * 0x47e1da2a executeBatch(address[],uint256[],bytes[])
+            */
+            bytes4(userOp.callData) == bytes4(0x47e1da2a) || bytes4(userOp.callData) == bytes4(0x18dfb3c7),
             "invalid callData"
         );
-        (address[] memory dest, bytes[] memory func) = abi.decode(userOp.callData[4:], (address[], bytes[]));
-        require(dest.length == func.length, "Paymaster: invalid callData length");
+        address[] memory dest;
+        bytes[] memory func;
+        if (bytes4(userOp.callData) == bytes4(0x47e1da2a)) {
+            (dest,, func) = abi.decode(userOp.callData[4:], (address[], uint256[], bytes[]));
+        } else {
+            (dest, func) = abi.decode(userOp.callData[4:], (address[], bytes[]));
+        }
 
+        require(dest.length == func.length, "Paymaster: invalid callData length");
         address _destAddress = address(0);
+        bool checkAllowance = false;
         for (uint256 i = 0; i < dest.length; i++) {
             address destAddr = dest[i];
             require(isSupportToken(token), "Paymaster: token not support");
-            if (destAddr == token) {
+            // check it contains approve operation, 0x095ea7b3 approve(address,uint256)
+            if (destAddr == token && bytes4(func[i]) == bytes4(0x095ea7b3)) {
                 (address spender, uint256 amount) = _decodeApprove(func[i]);
                 require(spender == address(this), "Paymaster: invalid spender");
-                require(amount >= tokenRequiredPreFund, "Paymaster: snot enough approve");
+                require(amount >= tokenRequiredPreFund, "Paymaster: not enough approve");
+                checkAllowance = true;
+                break;
             }
-            require(destAddr > _destAddress, "Paymaster: duplicate");
-            _destAddress = destAddr;
         }
+        require(checkAllowance, "no approve found");
         // callGasLimit
         uint256 callGasLimit = dest.length * _SAFE_APPROVE_GAS_COST;
         require(userOp.callGasLimit >= callGasLimit, "Paymaster: gas too low for postOp");
     }
 
     function _decodeApprove(bytes memory func) private pure returns (address spender, uint256 value) {
-        // 0x095ea7b3 approve(address,uint256)
         // 0x095ea7b3   address  uint256
         // ____4_____|____32___|___32__
-
-        require(bytes4(func) == bytes4(0x095ea7b3), "invalid approve func");
         assembly {
             spender := mload(add(func, 36)) // 32 + 4
             value := mload(add(func, 68)) // 32 + 4 +32

diff --git a/script/CreateWalletEntryPointPaymaster.s.sol b/script/CreateWalletEntryPointPaymaster.s.sol
@@ -102,10 +102,14 @@ contract CreateWalletEntryPointPaymaster is Script {
 
         tokenAddressList[0] = address(payToken);
 
+        uint256[] memory values = new uint256[](1);
+        values[0] = 0;
+
         bytes[] memory tokenCallData = new bytes[](1);
         tokenCallData[0] = abi.encodeWithSignature("approve(address,uint256)", address(paymaster), 1000e6);
-        bytes memory callData =
-            abi.encodeWithSignature("executeBatch(address[],bytes[])", tokenAddressList, tokenCallData);
+        bytes memory callData = abi.encodeWithSignature(
+            "executeBatch(address[],uint256[],bytes[])", tokenAddressList, values, tokenCallData
+        );
         bytes memory paymasterAndData =
             abi.encodePacked(abi.encodePacked(address(paymaster)), abi.encode(address(payToken), uint256(1000e6)));
 

diff --git a/test/paymaster/ERC20PaymasterActiveWallet.t.sol b/test/paymaster/ERC20PaymasterActiveWallet.t.sol
@@ -72,7 +72,7 @@ contract ERC20PaymasterActiveWalletTest is Test, UserOpHelper {
         vm.stopPrank();
     }
 
-    function testActiveWalletUsingPaymaster() external {
+    function test_ActiveWalletUsingPaymaster() external {
         address sender;
         uint256 nonce;
         bytes memory initCode;
@@ -119,9 +119,14 @@ contract ERC20PaymasterActiveWalletTest is Test, UserOpHelper {
         address[] memory tokenAddressList = new address[](1);
         tokenAddressList[0] = address(token);
 
+        uint256[] memory values = new uint256[](1);
+        values[0] = 0;
+
         bytes[] memory tokenCallData = new bytes[](1);
         tokenCallData[0] = abi.encodeWithSignature("approve(address,uint256)", address(paymaster), 1000e6);
-        callData = abi.encodeWithSignature("executeBatch(address[],bytes[])", tokenAddressList, tokenCallData);
+        callData = abi.encodeWithSignature(
+            "executeBatch(address[],uint256[],bytes[])", tokenAddressList, values, tokenCallData
+        );
         paymasterAndData =
             abi.encodePacked(abi.encodePacked(address(paymaster)), abi.encode(address(token), uint256(1000e6)));
 
@@ -144,4 +149,83 @@ contract ERC20PaymasterActiveWalletTest is Test, UserOpHelper {
         soulWallet = ISoulWallet(sender);
         assertEq(soulWallet.isOwner(ownerAddr.toBytes32()), true);
     }
+
+    function test_ActiveWalletWithoutApprove() external {
+        address sender;
+        uint256 nonce;
+        bytes memory initCode;
+        bytes memory callData;
+        uint256 callGasLimit;
+        uint256 verificationGasLimit;
+        uint256 preVerificationGas;
+        uint256 maxFeePerGas;
+        uint256 maxPriorityFeePerGas;
+        bytes memory paymasterAndData;
+        bytes memory signature;
+
+        nonce = 0;
+
+        (address trustedManagerOwner,) = makeAddrAndKey("trustedManagerOwner");
+        TrustedModuleManager trustedModuleManager = new TrustedModuleManager(trustedManagerOwner);
+        TrustedPluginManager trustedPluginManager = new TrustedPluginManager(trustedManagerOwner);
+        SecurityControlModule securityControlModule =
+            new SecurityControlModule(trustedModuleManager, trustedPluginManager);
+
+        bytes[] memory modules = new bytes[](1);
+        modules[0] = abi.encodePacked(securityControlModule, abi.encode(uint64(2 days)));
+        bytes[] memory plugins = new bytes[](0);
+
+        bytes32 salt = bytes32(0);
+        bytes32[] memory owners = new bytes32[](1);
+        owners[0] = ownerAddr.toBytes32();
+        DefaultCallbackHandler defaultCallbackHandler = new DefaultCallbackHandler();
+        bytes memory initializer = abi.encodeWithSignature(
+            "initialize(bytes32[],address,bytes[],bytes[])", owners, defaultCallbackHandler, modules, plugins
+        );
+        sender = soulWalletFactory.getWalletAddress(initializer, salt);
+        // send wallet with testtoken
+        token.sudoMint(address(sender), 1000e6);
+        bytes memory soulWalletFactoryCall = abi.encodeWithSignature("createWallet(bytes,bytes32)", initializer, salt);
+        initCode = abi.encodePacked(address(soulWalletFactory), soulWalletFactoryCall);
+
+        verificationGasLimit = 2000000;
+        preVerificationGas = 500000;
+        maxFeePerGas = 10 gwei;
+        maxPriorityFeePerGas = 10 gwei;
+        callGasLimit = 3000000;
+
+        address[] memory tokenAddressList = new address[](1);
+        tokenAddressList[0] = address(token);
+
+        uint256[] memory values = new uint256[](1);
+        values[0] = 0;
+
+        bytes[] memory tokenCallData = new bytes[](1);
+        tokenCallData[0] = abi.encodeWithSignature("allowance(address,uint256)", address(paymaster), 1000e6);
+        callData = abi.encodeWithSignature(
+            "executeBatch(address[],uint256[],bytes[])", tokenAddressList, values, tokenCallData
+        );
+        paymasterAndData =
+            abi.encodePacked(abi.encodePacked(address(paymaster)), abi.encode(address(token), uint256(1000e6)));
+
+        UserOperation memory userOperation = UserOperation(
+            sender,
+            nonce,
+            initCode,
+            callData,
+            callGasLimit,
+            verificationGasLimit,
+            preVerificationGas,
+            maxFeePerGas,
+            maxPriorityFeePerGas,
+            paymasterAndData,
+            signature
+        );
+
+        userOperation.signature = signUserOp(userOperation, ownerKey);
+        vm.expectRevert(
+            abi.encodeWithSelector(bytes4(keccak256("FailedOp(uint256,string)")), 0, "AA33 reverted: no approve found")
+        );
+        bundler.post(entryPoint, userOperation);
+    }
 }