Potential DoS in `removeMinter()` in `ERC20PermitPermissionedMint.sol`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/8073cc4ca44d9162873494f1cd9915a5d7b46f2b/src/ERC20/ERC20PermitPermissionedMint.sol#L76

Vulnerability details

Proof of concept

The code loops through the minters_array until it finds the minter_address to be removed. Every new minter will be pushed to the end of the minters_array array. In the case that there were many minters added already and an owner adds a wrong address as the minter it is possible that the for loop takes too much gas and the transaction reverts because it will hit the block gas limit.

Impact

The impact is a potential Denial of Service when trying to remove a wrong or a malicious minter.

Recommendation

Remove the minters_array since it’s not used for any critical logic in the code or instead implement the same mechanism to pop a minter from the end of the array as in OperatorRegistry.sol popValidators()

[FortisFortuna]

Technically correct, but in practice, the number of minters will always remain low. If it becomes an issue, we can designate one minter as a "pre-minter" that has a batch of tokens minted to it beforehand, then auxiliary contracts can connect to that instead of ERC20PermitPermissionedMint.sol instead.

[joestakey]

Duplicate of #12