Omniscia Nori Audit

LockedNORI Manual Review Findings

LockedNORI Manual Review Findings

LNO-01M: Inexistent Validation of Deposit Role

Description:

The _depositFor contains commented out code meant to validate the TOKEN_GRANTER_ROLE as the token's documentation indicates, however, no such validation is performed.

Example:

contracts/LockedNORI.sol
419/**
420 * @dev Wraps minting of wrapper token and grant setup.
421 * @param amount uint256 Quantity of `_bridgedPolygonNori` to deposit
422 * @param userData CreateTokenGrantParams or DepositForParams
423 * @param operatorData bytes extra information provided by the operator (if any)
424 *
425 * If `startTime` is zero no grant is set up.
426 * Satisfies situations where funding of the grant happens over time.
427 */
428function _depositFor(
429  uint256 amount,
430  bytes calldata userData,
431  bytes calldata operatorData
432) internal returns (bool) {
433  // require(
434  //   hasRole(TOKEN_GRANTER_ROLE, tx.origin), // todo figure out how to make this safe
435  //   "lNORI: requires TOKEN_GRANTER_ROLE"
436  // );
437  DepositForParams memory params = abi.decode(userData, (DepositForParams)); // todo error handling
438  // If a startTime parameter is non-zero then set up a schedule
439  if (params.startTime > 0) {
440    _createGrant(amount, userData);
441  }
442  super._mint(params.recipient, amount, userData, operatorData);
443  return true;
444}

Recommendation:

We advise the validation to be performed properly by utilizing the from or operator arguments of the tokensReceived hook which should be validated to hold the said role. The usage of tx.origin is strongly advised against as it would render the function fully susceptible to a re-entrancy attack during the execution of the _mint hook prior to _depositFor's conclusion.

Alleviation:

Validation of parameter validity is now correctly performed within the _createGrant function thereby alleviating this exhibit.

LNO-02M: Inexistent Validation of Grant Data

Description:

The _depositFor function does not validate the grant data it accepts.

Example:

contracts/LockedNORI.sol
428function _depositFor(
429  uint256 amount,
430  bytes calldata userData,
431  bytes calldata operatorData
432) internal returns (bool) {
433  // require(
434  //   hasRole(TOKEN_GRANTER_ROLE, tx.origin), // todo figure out how to make this safe
435  //   "lNORI: requires TOKEN_GRANTER_ROLE"
436  // );
437  DepositForParams memory params = abi.decode(userData, (DepositForParams)); // todo error handling
438  // If a startTime parameter is non-zero then set up a schedule
439  if (params.startTime > 0) {
440    _createGrant(amount, userData);
441  }
442  super._mint(params.recipient, amount, userData, operatorData);
443  return true;
444}

Recommendation:

We advise some form of validation to be introduced, at least by ensuring that the startTime is in the future and the endTime is beyond the startTime, to guarantee proper grant creations.

Alleviation:

The claimedAmount member is now incremented for the sender instead of the recipient of the transfer thereby ensuring the code block operates correctly.

LNO-03M: Inexplicable Accumulation of Withdrawal Recipient

Description:

The withdrawTo function permits a user to unlock their tokens and increase the claimedAmount of a target user. If the target user has an active schedule, this will lead to the tokens unvested by the original sender to be permanently locked in the contract.

Example:

contracts/LockedNORI.sol
224/**
225 * @notice Unwrap BridgedPolygonNORI tokens and makes them available for use in the BridgedPolygonNORI contract
226 * @dev This function burns `amount` of wrapped tokens and withdraws them to the corresponding {BridgedPolygonNORI}
227 * tokens.
228 *
229 * ##### Requirements:
230 * - Can only be used when the contract is not paused.
231 */
232function withdrawTo(address account, uint256 amount) external returns (bool) {
233  TokenGrant storage grant = _grants[account];
234  super._burn(_msgSender(), amount, "", "");
235  _bridgedPolygonNori.send(
236    // solhint-disable-previous-line check-send-result, because this isn't a solidity send
237    account,
238    amount,
239    ""
240  );
241  grant.claimedAmount += amount;
242  emit TokensClaimed(account, amount);
243  return true;
244}

Recommendation:

We advise this trait to be evaluated and potentially prohibited.

Alleviation:

The Nori team adjusted the codebase to instead accumulate the claimedAmount on the sender of the withdrawTo function call, thereby properly increasing the amount of tokens they have claimed in contrast to the original implementation which incorrectly incremented the recipient's claimedAmount. As a result, we consider this exhibit adequately dealt with.

LNO-04M: Inexplicable Dual Schedule Creation

Description:

The _createGrant function will lead to both a vestingSchedule and a lockupSchedule being created for a particular grant with the same amount provided to the contract.

Example:

contracts/LockedNORI.sol
446/**
447 * @dev Sets up a vesting + lockup schedule for recipient (implementation).
448 *
449 * This will be invoked via the `tokensReceived` callback for cases
450 * where we have the tokens in hand at the time we set up the grant.
451 *
452 * It is also callable externally (see `grantTo`) to handle cases
453 * where tokens are incrementally deposited after the grant is established.
454 */
455function _createGrant(uint256 amount, bytes memory userData) internal {
456  CreateTokenGrantParams memory params = abi.decode(
457    userData,
458    (CreateTokenGrantParams)
459  );
460  require(
461    address(params.recipient) != address(0),
462    "Recipient cannot be zero address"
463  );
464  require(!_grants[params.recipient].exists, "lNORI: Grant already exists");
465  TokenGrant storage grant = _grants[params.recipient];
466  grant.grantAmount = amount;
467  grant.originalAmount = amount;
468  grant.exists = true;
469  if (params.vestEndTime > params.startTime) {
470    require(
471      params.vestCliff1Amount >= params.unlockCliff1Amount ||
472        params.vestCliff2Amount >= params.unlockCliff2Amount,
473      "lNORI: unlock cliff < vest cliff"
474    );
475    grant.vestingSchedule.totalAmount = amount;
476    grant.vestingSchedule.startTime = params.startTime;
477    grant.vestingSchedule.endTime = params.vestEndTime;
478    grant.vestingSchedule.addCliff(
479      params.cliff1Time,
480      params.vestCliff1Amount
481    );
482    grant.vestingSchedule.addCliff(
483      params.cliff2Time,
484      params.vestCliff2Amount
485    );
486  }
487  grant.lockupSchedule.totalAmount = amount;
488  grant.lockupSchedule.startTime = params.startTime;
489  grant.lockupSchedule.endTime = params.unlockEndTime;
490  grant.lockupSchedule.addCliff(params.cliff1Time, params.unlockCliff1Amount);
491  grant.lockupSchedule.addCliff(params.cliff2Time, params.unlockCliff2Amount);
492  emit TokenGrantCreated(
493    params.recipient,
494    amount,
495    params.startTime,
496    params.vestEndTime,
497    params.unlockEndTime
498  );
499}

Recommendation:

We advise this trait of the system to be revised and only either of the two schedule types to be created for a user per deposit.

Alleviation:

The code now evaluates that the recipient of a mint operation has an active grant in place thereby alleviating this exhibit. We should note that a carefully crafted grant assigned to the recipient could in theory cause a compromisation of the system, however, the immediate withdrawal of tokens is no longer possible.

LNO-05M: Inexplicable Functionality of Arbitrary Mints

Description:

The _depositFor function will simply mint locked bridged NORI tokens to the recipient in case the startTime has been set as zero, a trait that allows the TOKEN_GRANTER_ROLE to extract all NORI tokens present in the contract at will.

Example:

contracts/LockedNORI.sol
437DepositForParams memory params = abi.decode(userData, (DepositForParams)); // todo error handling
438// If a startTime parameter is non-zero then set up a schedule
439if (params.startTime > 0) {
440  _createGrant(amount, userData);
441}
442super._mint(params.recipient, amount, userData, operatorData);
443return true;

Recommendation:

We advise this trait of the system to be either justified or removed from the codebase, the latter of which we advise as it is a point of significant centralization.

Alleviation:

The sender of a transfer is now properly validated in the tokensReceived hook thereby alleviating this exhibit.

Prev

BridgedPolygonNORI.sol (BPN-M)

Next

BridgedPolygonNORI.sol (BPN-C)

On this page