Mento A-2

Reference: Emission.sol#L46-49

The Emission contract includes an onlyOwner setter function for the token. If this token is changed mid-cycle, the emission schedule will be disrupted, counting previously emitted tokens as emitted for the new token and potentially bricking the programmed amounts of governance token supply for any given time.

Remediations to Consider

Consider allowing this function to only set the token contract once.

Contracts like MentoToken.sol, Emission.sol, and Airgrab.sol have initializers and/or setters that are used to set values after deployment. This is due to circular dependencies where they each need the address of another contract that may yet be deployed in the factory contract’s createGovernance() call. However, there is a way to know the address of a contract before it is deployed, since when deploying a contract using the CREATE opcode, it will deterministically deploy the contract with an address based on the contract’s nonce value. Since createGoverance() will only be called once, these values are known and the addresses of the contracts to be deployed can be predicted in advance and used in the constructors of other contracts before their deployment, provided no calls are made to them before they are deployed.

Remediations to Consider

Remove unnecessary setters and initializers by predicting the required addresses before they are deployed. This will reduce the bytecode size of the contracts as well as remove the perception that these contracts are upgradable/mutable.

In LockingBase.sol’s getLock()function, a call is made to divUp() to get the lockSlope. The lockAmount is passed in as the a parameter which gets subtracted by 1 before it is divided.

function divUp(uint96 a, uint96 b) internal pure returns (uint96) {
  return ((a - 1) / b) + 1;
}

Reference: LockingBase#L219-L221

If lockSlope is 0, then the function will revert with an underflow. The case where the lockSlope can be set to zero is if the amountMultiplied is less than the ST_FORMULA_DIVIDER, which can occur if the cliff is set to zero, the slopePeriod is equal to the minSlopePeriod, which is 1, and the amount is sufficiently small, ≤ 5.

function getLock(
  uint96 amount,
  uint32 slopePeriod,
  uint32 cliff
) public view returns (uint96 lockAmount, uint96 lockSlope) {
  require(cliff >= minCliffPeriod, "cliff period < minimal lock period");
  require(slopePeriod >= minSlopePeriod, "slope period < minimal lock period");

  uint96 cliffSide = (uint96(cliff - uint32(minCliffPeriod)) * (ST_FORMULA_CLIFF_MULTIPLIER)) /
    (MAX_CLIFF_PERIOD - uint32(minCliffPeriod));
  uint96 slopeSide = (uint96((slopePeriod - uint32(minSlopePeriod))) * (ST_FORMULA_SLOPE_MULTIPLIER)) /
    (MAX_SLOPE_PERIOD - uint32(minSlopePeriod));
  uint96 multiplier = cliffSide + (slopeSide) + (ST_FORMULA_CONST_MULTIPLIER);

  uint256 amountMultiplied = uint256(amount) * uint256(multiplier);
  lockAmount = uint96(amountMultiplied / (ST_FORMULA_DIVIDER));
  lockSlope = divUp(lockAmount, slopePeriod);
}

Reference: LockingBase#L200-L221

Remediations to Consider

This case should be handled by a custom error, or the minimum amount to lock should be sufficiently high to prevent this underflow from occurring.

References: Emission.sol#L46-49, Emission.sol#L55-58, LockingBase.sol#L148, MentoToken.sol#L31-53

Consider adding sanity checks for all address setters to avoid misconfigurations with address(0) as a value.

Reference: Airgrab.sol#L131

In the Airgrab.sol contract, the modifier canClaim() unnecessarily casts the input parameter amount to uint256, even though it is already a uint256 variable. Consider removing this unnecessary typecast.

Reference: Airgrab.sol#L28-29, LockingBase.sol#L18-19

Both Airgrab and LockingBase contracts declare constant variables MAX_CLIFF_PERIOD and MAX_SLOPE_PERIOD. Consider moving these declarations to the shared imported interface ILocking.sol to keep one source of truth for these values.

Reference: Airgrab.sol#L66, LockingBase.sol#L190

The treasury variable, in Airgrab, is set in the initialize() function, used as a workaround for the circular dependencies with the Token and LockingContract. However, this value does not need to be initialized after deployment. Consider making it immutable and setting it in the constructor instead.

There are some comments that have spelling errors:

• “cliam” should be spelled “claim”

* @dev Check if the account can cliam

Reference: Airgrab.sol#L116

• “incldued” should be spelled “included”

* if the account hasn't already claimed and if it's incldued

Reference: Airgrab.sol#L118

• “MerkeTree” should be spelled “MerkleTree”

* @notice This contract implements a token airgrab gated by a MerkeTree and KYC using fractal.

Reference: Airgrab.sol#L18

• “scheduledRemainigSupply” should be spelled “scheduledRemainingSupply”

uint256 scheduledRemainigSupply = (TOTAL_EMISSION_SUPPLY * (positiveAggregate - negativeAggregate)) / SCALER;

Reference: Emission.sol#L100

Throughout the codebase, require statements are used, which revert with a set static string. For solidity versions 0.8.4+, custom errors can be used, which can return dynamic values to give more insight into what caused the error, as well as cost less gas on average.

Remediations to Consider

Replace all require statements with conditions that revert with a custom error containing relevant error information, in order to make it more clear why an error is occurring to the user as well as save slightly on gas costs.

MentoToken.sol inherits from both ERC20, and ERC20Burnable, however, ERC20Burnable also inherits ERC20.

contract MentoToken is ERC20, ERC20Burnable {

Reference: MentoToken.sol#L12

abstract contract ERC20Burnable is Context, ERC20 {

Reference: ERC20Burnable.sol#L14

Remediations to Consider

Remove the ERC20 dependency and only inherit from ERC20Burnable to prevent duplicate inheritance.

In Airgrab.sol, there are reentrancy guards on the functions claim() and drain(). However, there is no path for a reentrancy attack to occur. claim() properly follows the checks effects and interactions pattern by setting claimed to true for the sender before making an external call to the locking contract. Since the canClaim modifier is run and requires claimed for the sender to be false, there is no way to claim multiple times, even if the locking contract was malicious. For drain(), there would be no benefit to reenter the call as it is just transferring the contract’s balance of tokens to the treasury contract, and no intermediate state can be manipulated.

Remediations to Consider

Remove the reentrancy guards on claim() and drain() and remove it as a dependency, to reduce the gas costs of calling these functions and reduce the deployment cost of the contract.