NewOrderDAO

In the contracts DisbursementCliff and OneWaySwap the return value of some external transfer calls are not checked. Several tokens do not revert in case of failure and return false. If that happened, for example in the DisbursementCliff contract, the withdrawnTokens state variable would be incorrectly updated and the calculation of the amount of vested tokens would be wrong. It is also considered a best practice to check the return value of a ERC20.transfer() call.

Code Location

DisbursementCliff.sol

DisbursementCliff.sol

function withdraw(address _to, uint256 _value)
    public
    isReceiver
{
    uint maxTokens = calcMaxWithdraw();
    if (_value > maxTokens){
      revert("Withdraw amount exceeds allowed tokens");
    }
    withdrawnTokens += _value;
    token.transfer(_to, _value);
}

/// @dev Transfers all tokens to multisig wallet
function walletWithdraw()
    public
    isWallet
{
    uint balance = token.balanceOf(address(this));
    withdrawnTokens += balance;
    token.transfer(wallet, balance);
}

one-way-swap.sol

one-way-swap.sol

function swap(uint256 amount) 
    public
    whenNotPaused
{
    oldToken.transferFrom(msg.sender, burnAddress, amount);
    newToken.transfer(msg.sender, amount);
}

function burn(uint256 amount, string memory why)
    public
    whenNotPaused
{
    oldToken.transferFrom(msg.sender, burnAddress, amount);
    emit Burned(msg.sender, amount, why);
}

one-way-swap.sol

function walletWithdraw(ERC20 token, uint256 amount, address destination)
    public
    onlyOwner
{
    token.transfer(destination, amount);
}

SOLVED: NewOrderDAO team now makes use of SafeERC20.safeTransfer() and SafeERC20.safeTransferFrom() in all their token transfers.

The constructor of the OneWaySwap contract is missing address validation. Every address should be validated and checked that is different from zero. This is also considered a best practice.

one-way-swap.sol

constructor(ERC20 oldToken_, ERC20 newToken_, address burnAddress_)
{
    oldToken = oldToken_;
    newToken = newToken_;        
    burnAddress = burnAddress_;
    _pause();
}

SOLVED: NewOrderDAO team added the zero address checks.

Solidity compiler version 0.8.3, 0.8.4 and 0.8.9 fixed important bugs in the compiler. The version 0.8.3 set in the truffle-config.js file of the GovernanceTokenV2 project is missing all these fixes:

• 0.8.4
• 0.8.9

Code Location

GovernanceTokenV2.sol

pragma solidity ^0.8.3;

truffle-config.js

compilers: {
  solc: {
    version: "0.8.3", // Fetch exact version from solc-bin (default: truffle's version)
    // docker: true,        // Use "0.5.1" you've installed locally with docker (default: false)
    // settings: {          // See the solidity docs for advice about optimization and evmVersion
    //  optimizer: {
    //    enabled: false,
    //    runs: 200
    //  },
    //  evmVersion: "byzantium"
    // }
  },
},

SOLVED: NewOrderDAO team set in the truffle-config.js file the 0.8.9 version for the contracts GovernanceTokenV2 and OneWaySwap and the 0.6.12 version for the DisbursementCliff contract.

In the following contracts there are functions marked as public but they are never directly called within the same contract or in any of their descendants:

GovernanceTokenV2.sol

• newTimeLock() (GovernanceTokenV2.sol#70-85)
• balanceUnlocked() (GovernanceTokenV2.sol#158-160)

DisbursementCliff.sol

• withdraw() (DisbursementCliff.sol#67-77)
• walletWithdraw() (DisbursementCliff.sol#80-87)

one-way-swap.sol

• swap() (onewayswap.sol#32-38)
• burn() (onewayswap.sol#40-46)
• walletWithdraw() (onewayswap.sol#66-71)

SOLVED: NewOrderDAO team declared the mentioned functions as external to reduce the gas costs.

The contract TimeLockToken is not pausable/ownable. Even if this addition would add centralization it could be useful in case of an emergency, for example, the token could be paused in case of a cross-chain bridge hack.

SOLVED: NewOrderDAO team created a Pausable variant of the TimeLockToken contract called GovernanceTokenPausable. NewOrderDAO team will decide which variant to deploy.