pimlicolabs / erc20-paymaster Goto Github PK

hi @kristofgazso is this repository still supported? there are some contributions that I would like to add!

When I run Npx hardhat test, I encountered an unexpected error occurred:

TypeError: Cannot read properties of undefined (reading 'Assertion')

Hi,

There is not limit on the gas fee of one UserOP. A malicious user can compose an UserOP with very high preVerificationGas or maxPriorityFeePerGas, making the gas fee high enough to drain all the eth of the Paymaster. Note in this case the malicious user can bundle this UserOP with his own bundler, so that the profit is returned to himself. This means malicious user only lose the actual gas fee (which is low) of executing the UserOP.

This can be expolited for below purpose:

1. Attack Paymaster to make it unavailable: Drain all the eth from paymaster in a low cost.
2. Swap ERC20 token to eth in a favorable price: The token price is read from Oracle and is updated in postOp. It's possible that the price deviates from the actual price in extreme cases. The malicious user can compose an UserOP with high gas fee, use TokenPaymaster and bundle it with his own bundler. In this case the malicious user pay ERC20 token and get eth in a favorable price.

The above line can be improved with named imports

 import {BasePaymaster} from "@account-abstraction/contracts/core/BasePaymaster.sol"; 

Named imports can be applied to all imports.

Custom error codes can be used instead of strings.

https://github.com/pimlicolabs/erc20-paymaster-contracts/blob/60c3ea1c2d75069f9aa7f8d35bc08545a2edb34b/src/PimlicoERC20Paymaster.sol#L67-L70

Suggested Change:

error PimlicoERC20Paymaster__PriceMarkUpTooHigh();

if(_priceMarkup >= 120e4){
   revert PimlicoERC20Paymaster__PriceMarkUpTooHigh();
}

By using custom error codes instead of strings, less gas is used.

https://github.com/pimlicolabs/erc20-paymaster-contracts/blob/60c3ea1c2d75069f9aa7f8d35bc08545a2edb34b/src/PimlicoERC20Paymaster.sol#L8

Suggested Change:

import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"

Which can improve readability and also destruct only the necessary code.

This codebase currently uses magic numbers in certain areas. Replacing these magic numbers with descriptive constants will significantly improve code readability and maintainability.

1e6 magic number can be updated with PRICE_DENOMINATOR in the below line

The above lines can be updated with the below lines of code.

uint256 private constant DECIMAL_PRECISION = 8;

.
.
.

if (_tokenOracle.decimals() != DECIMAL_PRECISION || _nativeAssetOracle.decimals() != DECIMAL_PRECISION) { 
     revert OracleDecimalsInvalid(); 
 } 

some more magic numbers are used. It can be updated with constants

Also, Instead of declaring all variables as public. All variables can be declared as private and we can write external getter functions to get the values. Which can prove to be gas-efficient.

-  uint256 public constant PRICE_DENOMINATOR = 1e6; 
+ uint256 private constant PRICE_DENOMINATOR = 1e6; 

External Getter function

function getPriceDenominator() external view returns(uint256){
     return PRICE_DENOMINATOR;
}