Marblex

The changes made in commit [d4302c2d89369e99b154b4c3cee7f7cb727878f0](https://github.com/MarblexAudit/MBXToken-ERC20/commit/d4302c2d89369e99b154b4c3cee7f7cb727878f0) introduce a lack of access control on a critically privileged function: ```solidity function grantRole(bytes32 role, address account) public override(AccessControl, IAccessControl) { _beforeSetRole(role, account, true); super._grantRole(role, account); } ``` This introduction allows anyone to call the function `grantRole()` because the override calls `super._grantRole()` instead of `super.grantRole()`. Since the external `super.grantRole()` is where the access protection is located, this function can now be called by anyone. In turn, anyone can take on the `MINTER_ROLE` and `PAUSER_ROLE`.

The current implementation of the `MBXToken` only ensures that tokens cannot be transferred `from` an account that has been frozen. There are no checks on `msg.sender` (potentially distinct from `_msgSender()`, or the `to` account, all of which may be different addresses. - If tokens are transferred `to` a frozen account, then tokens that were previously in circulation become temporarily unavailable, while the account they were transferred to is left frozen. - In functions `permit()` and `_approve()`, the `spender` address is not checked to ensure the address is not frozen. This may allow a frozen account to send the owner's tokens to maliciously. - The `_msgSender()` executing any function call may be a frozen account. If a user has previously given approval to an account that becomes frozen, then the frozen account can still use the approval to transfer the tokens to any destination address. Additionally, a frozen `msg.sender` account can still use a valid signature in the `permit()` function on behalf of a non-frozen account. - In cases where the `tokenForwarder` contract is used to relay an address for `_msgSender()` that is distinct from the `msg.sender` interacting, then the `_msgSender()` can be a frozen account and still make calls to functions `transferFrom()` and `permit()` as described above.

The contract `MBXToken` has a code size that exceeds the limit of 24576 bytes. It is noted that there are several functions added which call existing inherited functions from `AccessControlEnumerable` with hardcoded input: - `addMinter()` - `addPauser()` - `renounceMinter()` - `renouncePauser()` - `revokeMinter()` - `revokePauser()` - `isMinter()` - `isPauser()` Functions `renounceMinter()`, `renouncePauser()`, `revokeMinter()`, `revokePauser()`, `isMinter()`, and `isPauser()` appear unneeded. The `MINTER_ROLE` and `PAUSER_ROLE` values are public and include compiler-generated getter functions. These getter functions can be used to return the bytes32 value representing each role, and then these roles can be used as input, along with the desired account address in functions `renounceRole()`, `revokeRole()`, and `hasRole()` respectively. The `bytes32` value of `MINTER_ROLE` is 0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6. The `bytes32` value of `PAUSER_ROLE` is 0x65d7a28e3265b37a6474929f336521b332c1681b933f6cb9f3376673440d862a Functions `addMinter()` and `addPauser()` include other checks through call to internal `_beforeGrant()` before calling `_grantRole()`. However, it is noted that `DEFAULT_ADMIN_ROLE` can still directly call function `grantRole()` to bypass these checks. Since the `owner` of the contract is necessarily also a `DEFAULT_ADMIN_ROLE` based on the logic of the contract, this makes the addition of the checks in `addMinter()` and `addPauser()` functions ineffectual.

The main use case of `TokenForwarder` appears to be its role as the `trustedForwarder` address used in the `ERC2771` inheritance of the `MBXToken` contract. If there are no other use cases of this contract, consider setting the destination address for the low-level `call` in the function `execute()` upon deployment of the contract, instead of letting the user determine the destination. With its current set up, users can sign any message for any `req.to` destination, and the contract will execute the call to that destination.

One or more parameters with `memory` data location are never modified in their functions and those functions are never called internally within the contract. Thus, their data location can be changed to `calldata` to avoid the gas consumption copying from calldata to memory. ```solidity=75 function submitTransaction(address _to, uint256 _value, bytes memory _data) public onlyOwner { ``` `submitTransaction` has memory location parameters: `_data`.

The `to` address is not validated before assignment or external calls, potentially allowing the use of the zero address and leading to unexpected behavior or vulnerabilities. For example, transferring tokens to a zero address can result in a permanent loss of those tokens.

The modifier `notFrozen()` can be reconstructed to save gas during deployment by calling an internal view function instead of directly calling a `require` statement. See an example of this implementation in the following OpenZeppelin contract: [https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L46](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L46). Code explicitly written in modifiers is copied in all other function instances in which the modifier is used within the contract. In turn, the overall size of the contract is increased. This can be prevented by instead using an internal view function for the required check, as is demonstrated in the link above. Note, however, that function calls in which this is used may cost a slight extra amount in gas each time if this revision is made.

Function `execute()` of contract `TokenForwarder` is payable, but there is no check that the included `msg.value` matches the input `req.value`. As a result, one of the following scenarios could occur: - `req.value` is 0, but a positive `msg.value` is included, resulting in native tokens left in the contract. - If the contract does retain any native tokens, either through the scenario above, or by any other means, then a user can provide a valid signed message that they create, with `req.value` specified as the amount left in the contract. In this case, the caller of `execute()` does not have to provide a `msg.value`, and whatever is left in the contract will be sent wherever the caller specified with their signed message.

- Function `acceptOwnership()` overridden in `MBXToken` includes a requirement that `pendingOwner()` is not `address(0)`. However, it is not possible for `address(0)` to call this function directly, and the `TokenForwarder` contract that may be used to change the return value of `_msgSender()` cannot send `address(0)` as the source address. This is because the recovered signer of the `ForwardRequest` is checked in the `ECDSA` library to be a nonzero address, and reverts if this is the case. Consequently, the check that the `pendingOwner()` is not `address(0)` is unnecessary and can be removed. - Internal function `_beforeGrant()` requires that the input `role` is not the `DEFAULT_ADMIN_ROLE`, however, this internal function is only called in functions `addMinter()` and `addPauser()` where the `role` is either `MINTER_ROLE` or `PAUSER_ROLE` respectively. Consequently, the check that the `role` is not the `DEFAULT_ADMIN_ROLE` is unnecessary and can be removed.

In the locations cited, the function called is inherited by the contract and can be referenced directly, without the use of the keyword `super`.