FastToken Distribution Contracts Security Review Report

The conditions at lines 126-127 are both inverted (but not their "reason", which is correct), making owner change executions revert when they should pass and vice versa. Since these requirements directly contradict the conditions imposed when the transaction was submitted (lines 92-93), this leaves the contract without any means to update owner addresses.

if (TransactionType.ChangeOwner == transaction.transactionType) {
    require(! isOwner[transaction.address1], 'address1 must be owner');
    require(isOwner[transaction.address2], 'address2 cannot be owner');
    uint256 index = _ownerIndex(transaction.address1);
    owners[index] = transaction.address2;
    isOwner[transaction.address1] = false;
    isOwner[transaction.address2] = true;
}

Invert conditions by respectively removing and adding the negation operator, so they match logically with the revert reason.

The following event signatures contain only transaction index information and don't show the address that invoked the action. Not having an invoker address logged in the event rises a possibility for phishing attacks on owner change mechanisms; as one can start a vote to change other owner's public key.

emit SubmitTransaction(txIndex);
emit ConfirmTransaction(_txIndex);
emit ExecuteTransaction(_txIndex);
emit RevokeConfirmation(_txIndex);

Improve the events to contain the caller's address as well.

The listed methods duplicate the functionality provided by the compiler for all public variables, which include a getter in the ABI, making them or the custom getters redundant.

function getOwners() public view returns (address[] memory) {
    return owners;
}
…
function getTransactions() public view returns (Transaction[] memory) {
    return transactions;
}

Change variable visibility from public to internal, or delete the getter method to save on gas.

Arrays and structs don't play well together in Solidity, but in most cases using a mapping solves the issue and offers efficiency gains.

address[] public owners;
mapping(address => bool) public isOwner;
uint256 public numberOfRequiredConfirmations;
 
// mapping from transaction index => owner index => bool
mapping(uint256 => mapping(uint256 => bool)) public isConfirmed;
 
Transaction[] public transactions;

Use a mapping instead of an array, add an index array if necessary.

The methods listed below are not being used locally, their visibility is excessive.

MultiSigWallet.sol

• submitTransaction():L82
• confirmTransaction():L101
• executeTransaction():L116
• revokeConfirmation():L141
• getOwners():L158
• getTransactionCount():L163
• getTransactions():L168

Change the visibility to external.

The pragma is defined with a caret (^), allowing any version higher than 0.8.0 to compile the code. For the branch that will contain the code to be deployed, it is recommended to use a fixed pragma (without caret), to ensure compilations are consistent across time and dev environments, and contract verification is made easier.

pragma solidity ^0.8.0;

Modify the pragma to enforce a version at least equal to the latest stable compiler version.

Owners are considered friendly unless compromised. In this case, hostile owners are able to sign transactions, making it seem like it can be executed, then frontrun the execution transaction with a cancel confirmation tx, getting below threshold and failing the execution.

This is only slightly different from grieving by refusing to sign, and is therefore rated only informational.

function revokeConfirmation(uint256 _txIndex)
    public
    onlyOwner
    transactionExists(_txIndex)
    notExecuted(_txIndex) {
    Transaction storage transaction = transactions[_txIndex];
 
    uint256 index = _ownerIndex(msg.sender);
    require(isConfirmed[_txIndex][index], 'transaction not confirmed');
 
    transaction.numberOfConfirmations -= 1;
    isConfirmed[_txIndex][index] = false;
 
    emit RevokeConfirmation(_txIndex);
}

To entirely avoid this scenario, at the expense of slower execution speed, introduce a period for confirmation and a period for revoke, then execute transactions only after revoke period.