PMERC20

Finding Description

The contract implements a freeze mechanism through the freeze() function and isNotFrozen modifier. However, the implementation in transferFrom() is incomplete:

function transferFrom(address from, address to, uint256 value) public override isNotFrozen(from) returns (bool) {    return super.transferFrom(from, to, value);}

The function only applies the isNotFrozen modifier to the from parameter (token owner) but not to msg.sender (the spender). This means that if a spender account is frozen, they can still use existing allowances to transfer tokens from other accounts.

This creates a significant gap in the freeze functionality, particularly in scenarios where:

1. Users grant allowances to accounts that later become compromised
2. Attackers obtain allowances through phishing campaigns
3. The protocol is required by jurisdictional authorities to prevent an account from interacting with the token.

Major stablecoins like USDC and USDT prevent frozen accounts from spending allowances, making this behavior inconsistent with established standards.

Impact Explanation

The impact is high as it significantly reduces the effectiveness of the freeze mechanism. When accounts are compromised through phishing or other attacks, administrators cannot fully prevent token movement by freezing the attacker's account. This can lead to continued unauthorized transfers and potential loss of user funds.

Likelihood Explanation

The likelihood is low as it requires specific conditions (compromised accounts with existing allowances) for the full impact to be realized.

Recommendation

Consider modifying the transferFrom() function to also check if the spender is frozen:

- function transferFrom(address from, address to, uint256 value) public override isNotFrozen(from) returns (bool) {+ function transferFrom(address from, address to, uint256 value) public override isNotFrozen(from) isNotFrozen(msg.sender) returns (bool) {    return super.transferFrom(from, to, value);}

Description

The PMERC20Upgradeable contract is vulnerable to front-running attacks on the freeze() function in blockchain networks with public mempools. When an owner submits a transaction to freeze an account, the target account can observe this pending transaction in the mempool and front-run it by submitting a transfer() transaction with higher gas fees to move their balance to another address before the freeze takes effect.

Since protocol will deploy this token on ethereum, polygon, avalanche etc. which has public mempool, this becomes important to be taken care of.

Recommendation

This is an inherent limitation of public mempool architectures where any transaction can be front-run. Consider using private mempools, commit-reveal schemes, or accept this as expected behavior given the transparency requirements of blockchain networks where token would be deployed on.

Finding Description

The PMERC20Upgradeable contract does not include a constructor that calls _disableInitializers(). According to OpenZeppelin's security recommendations, upgradeable contracts should disable initialization on their implementation contracts to prevent potential security issues.

Without this protection, the implementation contract itself could theoretically be initialized directly, though this typically doesn't lead to practical exploits in most deployment scenarios. The OpenZeppelin documentation explains that calling _disableInitializers() in the constructor "locks the contract, preventing any future reinitialization" and is "recommended to use this to lock implementation contracts that are designed to be called through proxies."

This follows OpenZeppelin's recommended security practices for upgradeable contracts. For more details, see the OpenZeppelin Initializable documentation.

Recommendation

Add a constructor to the contract that calls _disableInitializers():

contract PMERC20Upgradeable is Initializable, ERC20Upgradeable, Ownable2StepUpgradeable {+   /// @custom:oz-upgrades-unsafe-allow constructor+   constructor() {+       _disableInitializers();+   }    struct OrebitDecimalStorage {        uint8 _decimals;    }    // ... rest of contract

Description

The contract implements a custom burn() function that mimics the behavior of burnFrom() in OpenZeppelin's ERC20BurnableUpgradeable. The current implementation requires allowance approval and can burn tokens from any account, which is functionally equivalent to the standard burnFrom() method.

If the contract requires both direct burning (burn()) and allowance-based burning (burnFrom()), inheriting from ERC20BurnableUpgradeable would provide both functionalities.

Recommendation

Consider inheriting from ERC20BurnableUpgradeable to gain access to burnFrom() (burns tokens with allowance). Note: this would also require overriding the burn() function to cause it to revert.

Alternatively, consider updating the name of the function to burnFrom.

Additionally, we recommend adding a note in the natspec that this function is intentionally designed for the owner to burn shares of other accounts who have given the owner an allowance.

Finding Description

The PMERC20Upgradeable contract contains several instances where the super keyword is used unnecessarily. While some uses of super are appropriate others are redundant and can be simplified.

The unnecessary uses include:

1. Internal function calls: Using super._mint(), super._burn(), and super._spendAllowance() when these functions aren't overridden in the contract.

2. totalSupply() calls: Using super.totalSupply() when totalSupply() would suffice, since the contract doesn't override totalSupply().

3. balanceOf() calls: Using super.balanceOf() when balanceOf() would work fine, since the contract doesn't override balanceOf().

Recommendation

Simplify the code by removing unnecessary super calls. Additionally, consider replacing super altogether with the name of the contract containing the function to avoid ambiguity:

These changes improve code clarity and follow conventional Solidity practices for inheritance.difffunction mint(address account, uint256 value) external onlyOwner returns (uint256) {-   super._mint(account, value);+   _mint(account, value);-   return super.totalSupply();+   return totalSupply();}
function burn(address account, uint256 value) external onlyOwner returns (uint256) {    address spender = _msgSender();-   super._spendAllowance(account, spender, value);+   _spendAllowance(account, spender, value);-   super._burn(account, value);+   _burn(account, value);-   return super.totalSupply();+   return totalSupply();}
function balanceOfBatch(address[] calldata accounts) public view virtual returns (uint256[] memory) {    uint256 length = accounts.length;    uint256[] memory balances = new uint256[](length);    for (uint256 i = 0; i < length; i++) {-       uint256 bal = super.balanceOf(accounts[i]);+       uint256 bal = balanceOf(accounts[i]);        balances[i] = bal;    }    return balances;}

For the overridden transfer() and transferFrom() functions, consider using explicit contract names instead of super for clarity:

function transfer(address to, uint256 value) public override isNotFrozen(msg.sender) returns (bool) {-   return super.transfer(to, value);+   return ERC20Upgradeable.transfer(to, value);}
function transferFrom(address from, address to, uint256 value) public override isNotFrozen(from) returns (bool) {-   return super.transferFrom(from, to, value);+   return ERC20Upgradeable.transferFrom(from, to, value);}

Finding Description

The protocol currently plans to implement the Transparent Proxy Pattern for upgradeability. While this pattern remains secure and functional, OpenZeppelin now recommends migrating to UUPS (Universal Upgradeable Proxy Standard) proxies for new implementations due to several technical advantages.

The primary efficiency concern with Transparent proxies stems from the admin access control check that occurs on every function call:

function _beforeFallback() internal virtual override {    require(msg.sender != _admin(), "TransparentUpgradeableProxy: admin cannot fallback to proxy target");    super._beforeFallback();}

This check executes for every transaction, consuming additional gas regardless of whether upgrade functionality is being accessed. In contrast, UUPS proxies handle upgrade logic within the implementation contract itself, eliminating this overhead for regular function calls.

UUPS proxies offer several advantages over Transparent proxies:

• Reduced deployment costs: UUPS proxies are lighter weight since upgrade logic resides in the implementation
• Lower transaction costs: No admin check on every function call
• Flexible upgrade controls: Upgrade authorization can be customized per implementation
• Future-proofing: Upgrade functionality can be removed entirely if desired

Recommendation

Consider migrating to the UUPS proxy pattern for new deployments. The migration involves:

1. Inherit from UUPSUpgradeable in your implementation contracts:

import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
contract YourContract is UUPSUpgradeable {    function _authorizeUpgrade(address newImplementation)         internal         override         onlyOwner     {}}

2. Deploy using ERC1967Proxy instead of TransparentUpgradeableProxy:

import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
proxy = new ERC1967Proxy(    implementation,    abi.encodeWithSignature("initialize(...)"));

Description

The contract stores decimals as a mutable storage variable using the EIP-7201 pattern, requiring storage reads and assembly operations on every decimals() function call. Since decimals are typically fixed for a token and set only once during initialization, this value can either be hardcoded in the function return or made a compile-time constant to eliminate storage operations and significantly reduce gas costs for this frequently-called view function.

Recommendation

Consider either hardcoding the return value in the decimals() function e.g.:

function decimals() returns (uint8) { return 6; }

or making it a constant

uint8 public constant DECIMALS = 18