Subscription token distinction functionality is broken

The function _processNativePayment() will be called only in case the sub.erc20TokensValid is false, and this condition will only be met if the input _erc20Token address is set to address(0).

Therefore, the user could add a different token other than ERC20, such as an address of a malicious contract or any other standard. This could lead to unexpected behavior.

/// @notice Creates a subscription
function createSubscription(
  address _initiator, uint256 _amount, uint256 _interval,
  uint256 _validUntil, address _erc20Token
) external onlyFromEntryPointOrOwnerOrSelf {
  getKernelStorage().subscriptions[_initiator] = SubStorage({
    amount: _amount,
    validUntil: _validUntil,
    validAfter: block.timestamp,
    paymentInterval: _interval,
    subscriber: address(this),
    initiator: _initiator,
    erc20Token: _erc20Token,
    erc20TokensValid: _erc20Token == address(0) ? false : true
  });
}

/// @notice Processes a payment for the subscription
function processPayment() external nonReentrant {
  // Check whether it is a native payment or ERC20 or ERC721
  if (sub.erc20TokensValid) {
    _processERC20Payment(sub);
  } else {
    _processNativePayment(sub);
  }
}

A non-standard or malicious token contract registered as the ERC-20 payment token causes unexpected behavior when _processERC20Payment interacts with it. There is no allowlist or interface check to distinguish valid ERC-20s from arbitrary contracts.

Consider creating a requirement that clearly distinguishes between valid tokens, other types of tokens, and invalid tokens. A potential workaround would have been to add a supportsInterface call, but most ERC-20 tokens do not implement ERC165, making it unreliable.

Since the Initiator.sol contract is a singleton for every dApp, we propose adding a public mapping containing the addresses of all ERC-20 tokens that the dApp will be willing to accept as payment. There would also be whitelistTokenForPayment() and removeTokenForPayment() functions, callable only by the deployer of the Initiator contract. If the address of the token exists in the mapping, _processERC20Payment() will be called and _processNativePayment() otherwise.

This way, users could use valid tokens for payment, avoiding registering malicious tokens, any address, or any standards different from the accepted ones.

mapping(address => bool) public whitelistedAddresses;

event AddressAdded(address indexed _address);
event AddressRemoved(address indexed _address);

function whitelistTokenForPayment(address token) external onlyOwner {
  require(!whitelistedAddresses[_address], "Address is already whitelisted");
  whitelistedAddresses[_address] = true;
  emit AddressAdded(_address);
}

function removeTokenForPayment(address _address) external {
  require(whitelistedAddresses[_address], "Address is not whitelisted");
  delete whitelistedAddresses[_address];
  emit AddressRemoved(_address);
}

In processPayment:

- if (sub.erc20TokensValid) {
+ if (Initiator(_initiator).whitelistedAddresses[sub.erc20Token]) {
    _processERC20Payment(sub);
  } else {
    _processNativePayment(sub);
  }

Team Response: Acknowledged and fixed as suggested.