Token Sale & Comptroller Updates - Moonwell

During the code review, It has been observed that expired tokens are not included in the voting power. Although the expired tokens are not used in the contract depends on the protocol behavior that can directly affect voting. The voting power should be carefully designed with expired tokens.

Code Location

TokenSaleDistributor.sol

    function totalVotingPower(address user) public view returns (uint) {
        uint totalAllocatedToUser = totalAllocated(user);
        uint totalClaimedByUser = totalClaimed(user);
        return totalAllocatedToUser - totalClaimedByUser;
    }

SOLVED: The Moonwell Team solved this issue by deleting expired tokens from the code base.

Commit ID: Commit ID

During the code review, It has been noticed that the owner can delete all delegations from any account.

Code Location

TokenSaleDistributor.sol

    function resetAllocationsByUser(address[] memory recipients) external adminOnly {
        uint length = recipients.length;
        for (uint i; i < length; ++i) {
            uint votingPower = totalVotingPower(recipients[i]);
            _moveDelegates(delegates[recipients[i]], address(0), votingPower);
            delete allocations[recipients[i]];
        }
    }

RISK ACCEPTED: The Moonwell Team states that they will do this if they found a critical vulnerability after deployment, and they needed to claw back all tokens to prevent them from being stolen. In that case, they will deploy a new contract and re-add the claims as well. On the other hand, The Moonwell Team commented the feature on the code with the following PR.

Commit ID: Commit ID

Since i is an uint256, it is already initialized to 0. uint256 i = 0 reassigns the 0 to i which wastes gas.

Code Location

TokenSaleDistributor.sol

• Line 157: for (uint i = 0; i < recipients.length; i += 1) {

SOLVED: The Moonwell Team solved this issue by removing initialization.

Commit ID: Commit ID

In the loop below, the variable i is incremented using i+=. It is known that, in loops, using ++i costs less gas per iteration than i+=1.

Code Location

  TokenSaleDistributor.sol::31 => for (uint i; i < allocations[msg.sender].length; i += 1) {
  TokenSaleDistributor.sol::59 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::71 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::83 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::95 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::107 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::157 => for (uint i = 0; i < recipients.length; i += 1) {
  TokenSaleDistributor.sol::180 => for (uint i; i < recipients.length; i += 1) {

For example, based on the following test contract:

Test.sol

//SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i+=1) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

We can see the difference in gas costs:

SOLVED: The Moonwell Team solved this issue by following the above recommendation.

Commit ID: Commit ID

The solidity compiler will always read the length of the array during each iteration.

• If it is a storage array, this is an additional sload operation (100 additional extra gas (EIP-2929) for each iteration except the first)
• If it is a memory array, this is an extra mload operation (3 additional gas for each iteration except the first),
• If it is a calldata array, it is an extra calldataload operation (3 additional gas for each iteration except the first).

Code Location

  TokenSaleDistributor.sol::31 => for (uint i; i < allocations[msg.sender].length; i += 1) {
  TokenSaleDistributor.sol::59 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::71 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::83 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::95 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::107 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::157 => for (uint i = 0; i < recipients.length; i += 1) {
  TokenSaleDistributor.sol::180 => for (uint i; i < recipients.length; i += 1) {

SOLVED: The Moonwell Team solved this issue by caching arrays.

Commit ID: Commit ID

Shortening the revert strings to fit within 32 bytes will decrease deployment time gas and decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead to calculate memory offset, etc.

Code Location

TokenSaleDistributor.sol

    function becomeImplementation(TokenSaleDistributorProxy proxy) external {
        require(msg.sender == proxy.admin(), "Only proxy admin can change the implementation");
        proxy.acceptPendingImplementation();
    }

ACKNOWLEDGED: The Moonwell Team acknowledged this issue. This is a long-tail gas estimation issue, and the Moonwell Team would rather have clean error messages than fix gas on a revert the user could have detected.

Checking for non-zero transfer values can prevent an external call to save gas.

Code Location

TokenSaleDistributor.sol

• Line 189: function withdraw(uint amount) external adminOnly {

SOLVED: The Moonwell Team solved this issue by implementing amount check.

Commit ID: Commit ID

There are several loops in the contract that can eventually grow large enough to make future operations of the contract cost too much gas to fit in one block.

Code Location

TokenSaleDistributor.sol

  TokenSaleDistributor.sol::31 => for (uint i; i < allocations[msg.sender].length; i += 1) {
  TokenSaleDistributor.sol::59 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::71 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::83 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::95 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::107 => for (uint i; i < allocations[recipient].length; i += 1) {
  TokenSaleDistributor.sol::157 => for (uint i = 0; i < recipients.length; i += 1) {
  TokenSaleDistributor.sol::180 => for (uint i; i < recipients.length; i += 1) {

ACKNOWLEDGED: The Moonwell Team acknowledged this issue.

ABIEncoderV2 is enabled and using experimental features could be dangerous in live deployments. The experimental ABI encoder does not correctly handle non-integer values less than 32 bytes. This applies to bytesNN types, bool, enum and other types when they are part of an array or a struct and encoded directly from storage. This means these storage references have to be used directly inside abi.encode(...) as arguments in external function calls or in event data without prior assignment to a local variable. The types bytesNN and bool will result in corrupted data, while enum could cause an invalid revert.

Code Location

pragma experimental ABIEncoderV2

TokenSaleDistributor.sol - Line#4

SOLVED: The Moonwell Team solved this issue by deleting experimental check.

Commit ID: Commit ID

Additional gas optimizations and security checks are available for free when using newer versions of the compiler and optimizer.

• Safemath by default since 0.8.0 (maybe more efficient on gas than library-based safemath.)
• Low level inliner : As of 0.8.2, leads to cheaper gas runtime. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an extra 20-40 gas because of the extra 2 jump instructions and additional stack operations needed for function calls.
• Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases, used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stacking operations and extra deployment time costs. However, if the slot was already warm, this means additional cost of 100 gas along with the same unnecessary stack operations and extra deploy time costs.
• Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings with custom errors.

Code Location

TokenSaleDistributor.sol

pragma solidity 0.6.12;

SOLVED: The Moonwell Team solved this issue by updating pragma to 0.8.10.

Commit ID: Commit ID

Open TODOs can point to architecture or programming issues that still need to be resolved.

Code Location

TokenSaleDistributor.sol

    /// @notice EIP-20 token name for this token
    // TODO(lunar): validate this name.
    string public constant name = "vWELL";

SOLVED: The Moonwell Team solved this issue by deleting TODOs check.

Commit ID: Commit ID

There is a function declared as public that is never called internally within the contract. It is best practice to mark such functions as external instead, as this saves gas (especially in the case where the function takes arguments, as external functions can read arguments directly from calldata instead of having to allocate memory).

Code Location

TokenSaleDistributor.sol

    function delegate(address delegatee) public {
        return _delegate(msg.sender, delegatee);
    }

SOLVED: The Moonwell Team solved this issue by marking function as an external.

Commit ID: Commit ID

delegateBySig calls the Solidity ecrecover function directly to verify the given signatures. However, the ecrecover EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.

Although a replay attack seems not possible here since the nonce is increased each time, ensuring the signatures are not malleable is considered a best practice.

Code Location

TokenSaleDistributor.sol

    function delegateBySig(address delegatee, uint nonce, uint expiry, uint8 v, bytes32 r, bytes32 s) public {
        bytes32 domainSeparator = keccak256(abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainId(), address(this)));
        bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));
        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
        address signatory = ecrecover(digest, v, r, s);
        require(signatory != address(0), "VestingWell::delegateBySig: invalid signature");
        require(nonce == nonces[signatory]++, "VestingWell::delegateBySig: invalid nonce");
        require(block.timestamp <= expiry, "VestingWell::delegateBySig: signature expired");
        return _delegate(signatory, delegatee);
    }

SOLVED: The Moonwell Team solved this issue by changing implementation with ECDSA library.

Commit ID: Commit ID

Owner/governor only functions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.

Code Location

TokenSaleDistributor.sol

     function setVotingEnabled(bool enabled) external adminOnly {
         votingEnabled = enabled;
     }

SOLVED: The Moonwell Team solved this issue by adding event on the related function.

Commit ID: Commit ID