Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gasless feature review report #6

Open
jjyr opened this issue Jan 29, 2023 · 3 comments · Fixed by #7 · May be fixed by #8 or #3
Open

Gasless feature review report #6

jjyr opened this issue Jan 29, 2023 · 3 comments · Fixed by #7 · May be fixed by #8 or #3
Assignees

Comments

@jjyr
Copy link

jjyr commented Jan 29, 2023

This is a summary of the internal review of the gas-less feature.

We suggest some improvements to the contracts. And suggest a mechanism to mitigate the DOS risk, which can be implemented on the Godwoken.

Repository

Repository License

  • The original solidity contracts in eth-infinitism/account-abstraction are licensed under GPL-3.0 (BasePaymaster.sol), but this repo relicensed source code in MIT, which is violent the GPL-3.0 license.

Test coverage

Documentation

Contracts

EntryPoint

  1. Godwoken block producer node plays the role of bundler. But the full node's beneficial address isn't fixed. It may be changed. We should pay compensation to block.coinbase instead of GW_FULL_NODE, GW_FULLNODE should be removed.
  2. The calculation of outOpInfo.preOpGas differs from the original code. We should add a comment to explain it.
    • ours outOpInfo.preOpGas = preGas - gasleft();
    • origin outOpInfo.preOpGas = preGas - gasleft() + userOp.preVerificationGas;
  3. Compares paymasterDeadline with uninitialized deadline in L265
  4. Godwoken gasless feature use block producer as the bundler, so we do not simulate the user's operation. The simulateValidation can be removed.
  5. Consider including actualGasUsed instead of gasPrice in the UserOperationEvent https://github.com/eth-infinitism/account-abstraction/blob/develop/contracts/interfaces/IEntryPoint.sol#L25
  6. Should limit revert reason length of user operation AA-114 Prevent revert reason bombing of innerHandleOp() eth-infinitism/account-abstraction#178
  7. The comment is wrong. HandlePostOp is always executed whether the userOps is successful or failure https://github.com/godwokenrises/gasless-contract/blob/main/contracts/gasless/core/EntryPoint.sol#L81

BasePayMaster

  • BasePayMaster has an interval variable admin, duplicated to the Owner. We should remove the admin variable, use onlyOwner modifier to check permission.
  • Mixed uses admin and onlyOwner. Should remove admin.
  • AvailAddr should be implemented as an interface. Please move the availAddrs into a standalone interface, and add an example to demonstrate how to use it.
  • The name of availAddrs isn't clear. Rename it to whitelistContracts

PayMaster reputation

In ERC-4337, we don't know whether the paymaster or the user causes a failed tx. The bundler takes the risk by simulating the tx before submitting the tx to the miner.

In Godwoken gasless, the block producer is also a bundler. We must control the risk of being attacked by malicious users and paymasters.
The basic idea is to use a tiny gas to check the validity of user operation. The paymaster must decide whether the tx is valid - the paymaster takes the risk of being banned, or the tx is invalid - the block producer will simply reject the tx.

So we have these situations when a gas-less fails:

  1. Paymaster rejects the tx
    1. Tx doesn't pass the IPaymaster(paymaster).validatePaymasterUserOp
    2. Then the block producer should just reject the tx if the gas cost is less than PAY_MASTER_VALIDATE_GAS_LIMIT (PAY_MASTER_VALIDATE_GAS_LIMIT is a very small value)
    3. Ban pay master address if gas cost is greater than PAY_MASTER_VALIDATE_GAS_LIMIT
  2. PayMaster failure
    1. We should simply ban PayMaster if it raises unexpected errors, such as failing to process handlePostOp
  3. User operation failure
    1. The block producer should record each paymaster's total invalid gas.
    2. If a user passes validatePaymasterUserOp and still fails, the used gas should be accumulated to paymaster's total invalid gas
    3. The paymaster should be banned once total invalid gas reach a limit.
    4. This incentivize paymaster to check users' behavior before sponsoring.
  4. The block producer should be able to disable gas-less txs during DOS. We should add a config option to enable/disable the gas-less feature.
@RetricSu
Copy link
Collaborator

entrypoint:

  1. we actually going to add the preVerificationGas back to solve the cost coverage problem, see: - add preVerifycationGas for UserOperation #7
  2. In my understanding, we need the simulateValidation for user to find out what value should be put intoUserOperation.verifycationGas? eg, - https://github.com/godwokenrises/gasless-contract/pull/7/files#diff-4589b52dce51e42e1e54ddf093e3103220e943df9435365a65935fcd69448883R79-R84

the rest is all good. as for PayMaster reputation part, I haven't really thought about it, will see

@jjyr
Copy link
Author

jjyr commented Jan 31, 2023

  • We should explain why we charge more gas(verificationGas) in the comment.
  • Good point. Then we should keep simulateValidation for that use.
  • The comment is wrong. HandlePostOp is always executed whether the userOps is successful or fails

@Flouse
Copy link
Contributor

Flouse commented Jan 31, 2023

4. The block producer should be able to disable gas-less txs during DOS. We should add a config option to enable/disable the gas-less feature.

Do you mean implement this feature switch in the Solidity contract or Godwoken core/node?

@magicalne magicalne linked a pull request Feb 3, 2023 that will close this issue
@Flouse Flouse closed this as completed in #7 Mar 2, 2023
@Flouse Flouse reopened this Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment