AllianceBlockToken Manual Review Findings

ABT-01M: Inexistent Initialization Protection of Base Implementation

Type	Severity	Location
Language Specific		AllianceBlockToken.sol:L16

Description:

The contract is meant to be upgradeable yet does not properly protect its logic deployment from malicious initializations.

Example:

Copy
16function init(string memory name, string memory symbol, address admin, address minter, uint256 cap_) public initializer {
17    __ERC20_init_unchained(name, symbol);
18    __ERC20Snapshot_init_unchained();
19    __ERC20Permit_init(name);
20    __Pausable_init_unchained();
21    __AllianceBlockToken_init_unchained(cap_);
22    // We don't use __ERC20PresetMinterPauser_init_unchained to avoid giving permisions to _msgSender
23    _setupRole(DEFAULT_ADMIN_ROLE, admin);
24    _setupRole(MINTER_ROLE, admin);
25    _setupRole(PAUSER_ROLE, admin);
26    _setupRole(MINTER_ROLE, minter);
27}
28
29function __AllianceBlockToken_init_unchained(uint256 cap_) internal onlyInitializing {
30    require(cap_ > 0, "NXRA: cap is 0");
31    _cap = cap_;
32}

Recommendation:

We advise a constructor to be introduced that invokes the initializer modifier of the Initializable contract to prevent the base implementation from ever being initialized.

Alleviation (5bde836b59):

The base implementation is initialized via a direct invocation of init from the newly-introduced constructor of the contract. While this alleviates the exhibit, we advise the constructor to not invoke the init function with proper arguments and to instead contain the initializer modifier, thus preventing potential scams involving the base implementation token.

Alleviation (cba978de9f):

The constructor was revised according to our recommendation, solely "invoking" the initializer modifier and not actually initializing the token contract. As a result, we consider this exhibit fully alleviated.

ABT-02M: Insufficient Prevention of Contract Token Ownership

Type	Severity	Location
Logical Fault		AllianceBlockToken.sol:L46

Description:

The _transfer function has been overridden to prevent the contract itself (address(this)) from being in possession of a token balance, however, the check applied is insufficient as it is not validated during a _mint call.

Impact:

While this issue does not constitute an exploit, it is still a discrepancy in the system that can be caused by improper handling of the batchMint function by MINTER_ROLE users.

Example:

Copy
34// Update balance and/or total supply snapshots before the values are modified. This is implemented
35// in the _beforeTokenTransfer hook, which is executed for _mint, _burn, and _transfer operations.
36function _beforeTokenTransfer(
37    address from,
38    address to,
39    uint256 amount
40) internal virtual override(ERC20PresetMinterPauserUpgradeable, ERC20SnapshotUpgradeable, ERC20Upgradeable) {
41    super._beforeTokenTransfer(from, to, amount);
42}
43
44// Avoid direct transfers to this contract
45function _transfer(address from, address to, uint256 amount) internal override {
46    require(to != address(this), "NXRA: Token transfer to this contract");
47    super._transfer(from, to, amount);
48}

Recommendation:

We advise the check to be relocated to the _beforeTokenTransfer function that is already validated, ensuring that both _mint and _transfer operations do not increase the balance of the contract itself.

Alleviation (5bde836b591caa6c3dfd47b79f323317a26c8a0d):

The _transfer function overriddence has been omitted and its statements have been relocated to the _beforeTokenTransfer hook as advised.