Mento A-3

Note: This issue was found in Sherlock contest after this audit was performed, more details on this specific issue can be found here

An attacker can DoS the governance execution mechanism by making calls to LockingVotes.getPastTotalSupply gas expensive.

The total supply of veMento tokens is calculated in the Locking contract. The calculations take every liquidity lines stored in the

An attacker can create multiple small locks. For each lock, a line will be added to the total supply in the Locking contract.

Then, when a proposal is executed, the governor contract will call LockingVotes.getPastTotalSupply to get the veMENTO total supply at a specific block.

The issue is that this total supply is calculated from a set of lines. The more lines there are, the more expensive the call to Locking.getPastTotalSupply is.

As the attacker can register as much locks as he wants and that the amount of token for a lock can be low (104 in POC, nothing compared as token is 18 decimals), the attacker can make the calls to Locking.totalSupply and LockingVotes.getPastTotalSupply too gas-consuming that it doesn't fit in one block.

Impact HIGH Governance proposals can't be executed because of gas required.

In Locking.sol’s lock() function, a specified amount of Mento tokens can be locked in return for veMento for the duration of the lock period.

function lock(
  address account,
  address _delegate,
  uint96 amount,
  uint32 slopePeriod,
  uint32 cliff
) external override notStopped notMigrating returns (uint256) {
  require(amount > 0, "zero amount");
  require(cliff <= MAX_CLIFF_PERIOD, "cliff too big");
  require(slopePeriod <= MAX_SLOPE_PERIOD, "period too big");

  counter++;

  uint32 currentBlock = getBlockNumber();
  uint32 time = roundTimestamp(currentBlock);
  addLines(account, _delegate, amount, slopePeriod, cliff, time, currentBlock);
  accounts[account].amount = accounts[account].amount + (amount);

  // slither-disable-next-line reentrancy-events
  require(token.transferFrom(msg.sender, address(this), amount), "transfer failed");

  emit LockCreate(counter, account, _delegate, time, amount, slopePeriod, cliff);
  return counter;
}

Reference: Locking.sol#L44-L67

However, the inputted account and _delegate addresses are not validated to ensure they are not the zero address.

In the case where the _delegate is set to address(0), the mento tokens are locked for the set duration, but the veMento gained will be useless. Users may incorrectly assume that they can set delegate to address(0) so that they do not delegate to anyone else, rather than use their own address as the delegate as expected for this case.

In the case where account is address(0), then ownership of the locked Mento tokens would be given to the zero address which cannot withdraw the tokens, effectively burning the Mento tokens.

Remediations to Consider

Validate the account and _delegate input parameters to ensure they are not set to address(0) to prevent locking Mento tokens in the contract indefinitely, and miss-delegate of veMento.

The Locking contract supports migration of lock entries to another contract via the migrate function. In addition, MentoToken transfers are initially disabled and will be enabled at a later point when the contract gets unpaused. The logic to prevent transfers is implemented in the overridden _beforeTokenTransfer function:

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256 amount
  ) internal virtual override {
    super._beforeTokenTransfer(from, to, amount);

    require(to != address(this), "MentoToken: cannot transfer tokens to token contract");
    // Token transfers are only possible if the contract is not paused
    // OR if triggered by the owner of the contract
    // OR if triggered by the locking contract
    // OR if triggered by the emission contract
    require(
      !paused() || owner() == _msgSender() || locking == _msgSender() || emission == _msgSender(),
      "MentoToken: token transfer while paused"
    );
  }

Reference: MentoToken.sol#L116

According to this logic, transfers are only possible when the contract is unpaused or when triggered by the owner, emission, or locking contract.

But, if lock entries are migrated to a new Locking contract, this new contract is not allowed to trigger transfers anymore, since this new address is not updated in the MentoToken implementation. As a consequence, withdrawing from the new Locking contract would revert when the MentoToken is paused, leading to locked tokens until the MentoToken is unpaused.

Another consequence of migrating locks to a new contract is that calling lock would revert for the same reason, including a revert on the Airgrab.claim function.

Note that - after clarification with the Mento team - the severity of this issue is considered low, as it is very unlikely to utilize the Locking’s migrate function. If the Locking contract needs to be changed for any reason, the preferred method is to upgrade the contract rather than performing a migrate.

Remediation to Consider

Entirely remove migration capability from the Locking contract or alternatively, to maintain support for migration, consider allowing the locking address referenced in the MentoToken to be updated on migration.

The minimum allowed values for both the cliff and slope period can initially be set in the __LockingBase_init_unchained function during deployment. These values can be later adjusted via the owner-controlled functions setMinCliffPeriod and setMinSlopePeriod.

The require checks in the init function allow the min periods to be set to values equal to or less than the MAX_CLIFF_PERIOD **and **MAX_SLOPE_PERIOD:

//setting min cliff and slope
require(_minCliffPeriod <= MAX_CLIFF_PERIOD, "cliff too big");
require(_minSlopePeriod <= MAX_SLOPE_PERIOD, "period too big");

Reference: LockingBase.sol#L148-L149

However, in the respective setter functions, the require checks only allow values less than the maximum amounts, not equal to them.

function setMinCliffPeriod(uint32 newMinCliffPeriod) external notStopped notMigrating onlyOwner {
  require(newMinCliffPeriod < MAX_CLIFF_PERIOD, "new cliff period > 2 years");
  minCliffPeriod = newMinCliffPeriod;

  emit SetMinCliffPeriod(newMinCliffPeriod);
}

function setMinSlopePeriod(uint32 newMinSlopePeriod) external notStopped notMigrating onlyOwner {
  require(newMinSlopePeriod < MAX_SLOPE_PERIOD, "new slope period > 2 years");
  minSlopePeriod = newMinSlopePeriod;

  emit SetMinSlopePeriod(newMinSlopePeriod);
}

Reference: LockingBase.sol#L269-L281

Remediation to Consider

Change setMinCliffPeriod and setMinSlopePeriod to allow new values being equal to the configured MAX_CLIFF_PERIOD **and **MAX_SLOPE_PERIOD.

Currently in Locking.sol the MAX_CLIFF_PERIOD is less than the MAX_SLOPE_PERIOD:

uint32 constant MAX_CLIFF_PERIOD = 103;
uint32 constant MAX_SLOPE_PERIOD = 104;

Reference: LockingBase.sol#L18-L19

The calculation to determine the percent multiplier of locked Mento token to veMento, uses the cliff or slope period, divided by it’s respective max period:

uint96 cliffSide = (uint96(cliff) * ST_FORMULA_BASIS) / MAX_CLIFF_PERIOD;
uint96 slopeSide = (uint96(slopePeriod) * ST_FORMULA_BASIS) / MAX_SLOPE_PERIOD;
uint96 multiplier = cliffSide + slopeSide;

//@audit: notice the multiplier is capped at effectively 100%
if (multiplier > ST_FORMULA_BASIS) {
  multiplier = ST_FORMULA_BASIS;
}

uint256 amountMultiplied = uint256(amount) * uint256(multiplier);
lockAmount = uint96(amountMultiplied / (ST_FORMULA_BASIS));

Reference: LockingBase.sol#L219-L228

However, since the MAX_CLIFF_PERIOD is smaller, the weight of each cliff period is just under 1% more effective than if a user were to lock with a slope.

Remediations to Consider

Consider setting the max periods to be the same.

Note: This issue was found in Sherlock contest after this audit was performed, more details on this specific issue can be found here

We noticed the Airgrab contract was using an incorrect KYC credential and after the contest ended, we discussed this with the KYC provider and updated the contracts accordingly.

Old credential: level:plus;residency_not:ca,us

New credential: level:plus+liveness;citizenship_not:;residency_not:cd,cu,gb,ir,kp,ml,mm,ss,sy,us,ye

This issue is not about the countries that are put as placeholder, it is about the level of the credential which lacks liveness and citizenship_not:

Note: This issue was found in Sherlock contest after this audit was performed, more details on this specific issue can be found here

The flow to receiving voting power can be understood in simple terms as follows:

Users locks his MENTO and chooses a delegate-> received veMENTO which gives them(delegatee) voting power (there's cliff and slope at play too)

The veMENTO is not a standard ERC20 , it is depicted through "lines" , voting power declines ( ie. slope period) with time and with time you can withdraw more of your MENTO.

The edge case where the user will be withdrawing his entire locked MENTO amount and even then will be able to vote is as follows ->

1. User has locked his MENTO balance in the Locking.sol

2. The owner of the contract "stops" the contract for some emergency reason.

3. In this stopped state the user calls withdraw() which calls getAvailableForWithdraw() here https://github.com/sherlock-audit/2024-02-mento/blob/main/mento-core/contracts/governance/locking/Locking.sol#L97

4. Since the contract is stopped , the getAvailableForWithdraw will return the entire locked amount of the user as withdrawable

   function getAvailableForWithdraw(address account) public view returns (uint96) {
       uint96 value = accounts[account].amount;
       if (!stopped) {
         uint32 currentBlock = getBlockNumber();
         uint32 time = roundTimestamp(currentBlock);
         uint96 bias = accounts[account].locked.actualValue(time, currentBlock);
         value = value - (bias);
       }
       return value;
   

5. The user receives his entire locked amount in L101.

6. The owner "start()" the contract again

7. Since the user's veMENTO power was not effected by the above flow , there still exists veMENTO a.k.a voting power to the delegate, and the user's delegate is still able to vote on proposals (even when the user has withdrew everything).

Note: This issue was found in Sherlock contest after this audit was performed, more details on this specific issue can be found here

The startingPointWeek variable is responsible for maintaining internal timing consistency within the locking contract. This variable is initialized as follows in the GovernanceFactory:

uint32 startingPointWeek = uint32(Locking(lockingImpl).getWeek() - 1);
TransparentUpgradeableProxy lockingProxy = ProxyDeployerLib.deployProxy( // NONCE:7
  address(lockingImpl),
  address(proxyAdmin),
  abi.encodeWithSelector(
    lockingImpl.__Locking_init.selector,
    address(mentoToken), /// @param _token The token to be locked in exchange for voting power in form of veTokens.
    startingPointWeek, ///   @param _startingPointWeek The locking epoch start in weeks. We start the locking contract from week 1 with min slope duration of 1
    0, ///                   @param _minCliffPeriod minimum cliff period in weeks.
    1 ///                    @param _minSlopePeriod minimum slope period in weeks.
  )

Now, let's examine the function call trace during the initialization of this variable:

/**
* @notice Returns "current week" of the contract. The Locking contract works with a week-based time system
* for managing locks and voting power. The current week number is calculated based on the number of weeks passed
* since the starting point week. The starting point is set during the contract initialization.
*/
function getWeek() external view returns (uint256) {
  return roundTimestamp(getBlockNumber());
}
...
  /**
  * @notice Calculates the week number for a given blocknumber
  * @param ts block number
  * @return week number the block number belongs to
  */
  function roundTimestamp(uint32 ts) public view returns (uint32) { 
    if (ts < getEpochShift()) {
      return 0;
    }
    uint32 shifted = ts - (getEpochShift());
    return shifted / WEEK - uint32(startingPointWeek);
  }
...
function getBlockNumber() internal view virtual returns (uint32) {
  return uint32(block.number);
}

As we can observe, the getWeek function within LockingImpl returns the number of weeks elapsed since the gnosis block because currently startingPointWeek is 0 since it is not initialized yet. Let's assume it corresponds to "179" (Test setup that I will use from protocol tests is using this value so I will continue with it) and continue. The startingPointWeek within the locking contract is initialized with the value 179. Calculations involving the getWeek() function will operate correctly, as it deducts startingPointWeek when computing the system's internal week. Now lets check the setStartingPointWeek() function:

/**
* @notice Sets the starting point for the week-based time system
* @param newStartingPointWeek new starting point
*/
function setStartingPointWeek(uint32 newStartingPointWeek) public notStopped onlyOwner { 
  require(newStartingPointWeek < roundTimestamp(getBlockNumber()), "wrong newStartingPointWeek");
  startingPointWeek = newStartingPointWeek;

  emit SetStartingPointWeek(newStartingPointWeek);
}

As we can see from the require statement, setStartingPointWeek() will only permit newStartingPointWeek to be less than roundTimestamp(getBlockNumber()). Currently, when computing weeks, it subtracts startingPointWeek (initialized with 179). Therefore, if there have been 10 weeks since the initialization, setStartingPointWeek() can only accept numbers smaller than 10. This assertion is confirmed by the following Proof of Concept (POC):

function test_notAllowChangeWeek() public {
  vm.timeTravel(BLOCKS_WEEK * 10);
  vm.prank(address(governanceTimelock));
  locking.setStartingPointWeek(11);
}

Add the function provided above to LockIntegration.fuzz.t.sol and execute the test. You will notice that it reverts with "wrong newStartingPointWeek". This outcome aligns with the natspec comment observed during the deployment of Locking:

startingPointWeek, ///   @param _startingPointWeek The locking epoch start in weeks. We start the locking contract from week 1 with min slope duration of 1

So there is no problem so far, we can only change the startingPointWeek with values less than current internal week number. Let's change it and see what happens then (Again add it to LockIntegration.fuzz.t.sol):

function test_ChangeWeekBrokesSystem() public {
  vm.timeTravel(BLOCKS_WEEK * 10);

  uint32 currentWeek = locking.roundTimestamp(uint32(block.number));
  console.log("Internal week before update:", currentWeek);

  vm.prank(address(governanceTimelock));
  locking.setStartingPointWeek(9);

  currentWeek = locking.roundTimestamp(uint32(block.number));
  console.log("Internal week after update:", currentWeek);
}

Here is the console output:

[PASS] test_ChangeWeekBrokesSystem() (gas: 37271) Logs: Internal week before update: 11 Internal week after update: 181

As we can see internal week accounting is completely broken after a benign setStartingPointWeek() call. This issue arises because during the deployment of the proxy, the calculation setting startingPointWeek utilized the implementation contract's variable, which is (un)initialized to 0. Consequently, it computed the variable with reference to the gnosis block. However, upon calling setStartingPointWeek(), it utilizes the proxy's storage (now set to 179), causing it to calculate the variable with reference to the internal system time. As a result, instead of producing the expected impact, it essentially disrupts the entire week accounting in the contract, leading to the unlocking of all locks.

The contracts involved in vote escrowed Mento tokens, including Locking, LockingBase, LockingRelock, and LockingVotes and their interface contracts are missing Natspec comments, which is present in other contracts within the protocol. Natspec helps users and developers understand the intention of functions and the parameters.

Reference: GovernanceFactory.sol#L265

In GovernanceFactory.createGovernance, there is an assert statement after each deployment to verify that the pre-calculated address matches the actual deployed address.

However, this assert check is missing for the MentoGovernor deployment.

Remediation to Consider

For consistency and clarity sake, add an appropriate check to verify correct deployment of MentoGovernor.

Locking.sol allows users to lock their Mento tokens for a set period, and in return give them vote escrowed tokens that can be used to vote on proposals in Mento’s governance contract. The locking duration is based on block numbers rather than timestamps, which if the Celo network stops processing blocks, it would increase the expected duration that users Mento tokens would be locked for.

When locking Mento tokens in the Locking contract in return for veMento tokens, the amount of veMento received is determined by a percent multiplier that is calculated based on the locking duration, which is a combination of the cliff period and slope period set.

uint96 cliffSide = (uint96(cliff) * ST_FORMULA_BASIS) / MAX_CLIFF_PERIOD;
uint96 slopeSide = (uint96(slopePeriod) * ST_FORMULA_BASIS) / MAX_SLOPE_PERIOD;
uint96 multiplier = cliffSide + slopeSide;

//@audit: notice the multiplier is capped at effectively 100%
if (multiplier > ST_FORMULA_BASIS) {
  multiplier = ST_FORMULA_BASIS;
}

uint256 amountMultiplied = uint256(amount) * uint256(multiplier);
lockAmount = uint96(amountMultiplied / (ST_FORMULA_BASIS));

Reference: LockingBase.sol#L219-L228

If a cliff and slopePeriod were used that would bring the multiplier above 100%, then the multiplier would be capped, and the additional lock duration would not have any additional benefit other than maintaining a balance of veMento tokens for a longer duration. This may lead to users locking their Mento tokens for longer durations than they would otherwise.

The Mento team stated will notify users of this on their front end application, however users that integrate with this contract, or use it directly should be aware of this.