DreamTeam Smart Contracts Audit for Players’ Compensation

Coinfabrik was asked to audit the smart contracts for the DreamTeam system. The DreamTeam contracts implement a system that teams can use to compensate their members with tokens. In the first part, we will give a summary of our discoveries and follow them with the details of our findings.

Summary

The contracts audited are from the DreamTeam source code repository at commit c1efcabf8a56a4d0a1fa5185a095052c110bdd87.

The audited contracts are:

• SafeMath.sol: Safe integer calculations
• BasicStorage.sol: Multiple ownership contract base
• DreamTeamStorage.sol: Generic storage of primitive types, deployed separately uses BasicStorage for ownership.
• StorageInterface.sol: Interface for DreamTeamStorage
• TeamStorageController.sol: Functions to abstract high level types for storage in DreamTeamStorage
• TeamContracts.sol: Inherits TeamStorageController. Implements token payments to team members and general management of teams.
• ERC20TokenInterface.sol: ERC20 Token Interface
• TDTT.sol: Simple ERC20 Token implementation

These checks are performed:

• Misuse of the different call methods: call.value(), send() and transfer().
• Integer rounding errors, overflow, underflow and related usage of SafeMath functions.
• Old compiler version pragmas.
• Race conditions such as reentrancy attacks or front running.
• Misuse of block timestamps, assuming things other than them being strictly increasing.
• Contract softlocking attacks (DoS).
• Potential gas cost of functions being over the gas limit.
• Missing function qualifiers or misuse of them.
• Fallback functions with higher gas cost than a what a transfer or send call allows.
• Underhanded or erroneous code.
• Code and contract interaction complexity.
• Wrong or missing error handling.
• Overuse of transfers in a single transaction instead of using withdrawal patterns.
• Insufficient coverage of function input requirements.

None of the issues found was of critical severity.

Detailed findings

Medium severity

Ambiguous return value

There is a function called storageGetTeamMemberIndexByAddress, it returns the member index given a member address:

function storageGetTeamMemberIndexByAddress (address db, uint teamId, address memberAddress)
internal view returns(uint) {

    uint i = 0;

    address addr;

    do {

        addr = StorageInterface(db).getAddress(keccak256(Storage.teams, teamId, i));

        if (addr == memberAddress)

            return i;

        if (addr == 0x0)

            return 0x0;

        ++i;

    } while (true);

}

The problem with this function is that it does not distinguish between returning 0 because nothing was found and returning the member with index 0. This means that removeMember will remove the member index 0 if an invalid address is given:

function removeMember (uint teamId, address memberAccount) dreamTeamOnly public {


uint memberIndex = storageGetTeamMemberIndexByAddress(db, teamId, memberAccount);

uint payoutDate = storageGetTeamMemberPayoutDate(db, teamId, memberIndex);


(...)



// Actually delete team member from a storage

storageDeleteTeamMember(db, teamId, memberIndex);

TeamMemberRemoved(teamId, memberAccount);

}

Consider returning a tuple with an index and a bool that is false for the case when nothing is found.

This was fixed by the DreamTeam developers in commit dbb8182f40eb15e2cd73b6f65014a280057fbaf4.

Minor severity

Vesting mechanism does not emit Transfer events

The TDTT contract does not emit Transfer events when balances are assigned to token holders as a consequence of its vesting mechanism. This could make it difficult for someone to examine the historical token movements in a block explorer. In particular, it poses an inconvenience to users of applications that depend on these events to interact with the token contract.

Due to the nature of the mechanism, we recommend emitting a Transfer event once the balance of an address is updated according to the current block in the subFromBalance function.

Enhancements

These are presented as mere suggestions to the smart contract development team. They may help with the smart contract system overall.

Addresses can be declared as contracts

Lines like these:

ERC20TokenInterface(erc20TokenAddress).transfer(newDeployedTeamContracts,
ERC20TokenInterface(erc20TokenAddress).balanceOf(this)); // Move all funds to a new contract

Can be simplified by making the variable erc20TokenAddress an ERC20TokenInterface instead of an untyped address:

ERC20TokenInterface erc20TokenAddress;

Now the line above can be turned to:

erc20TokenAddress.transfer(newDeployedTeamContracts,erc20TokenAddress.balanceOf(this));
// Move all funds to a new contract

The same can be done with contracts that are not interfaces.

Add token transfer failsafe mechanisms

We recommend adding failsafe mechanisms to ensure tokens don’t get stuck indefinitely in any of the contracts involved. While this makes for a nice feature in user-facing smart contracts, in this case it may be a good idea to have it considering the existence of an upgrade mechanism. The failsafe could even work for the DreamTeam token itself in the TeamContracts contract with adequate restrictions.

An easy way to implement such a failsafe is to write a function like this one:

 function tokenFailsafeTransfer(address agent, uint tokens, ERC20 token_contract)
public ownersOnly {

// We use approve instead of transfer to minimize the possibility

// of getting them stuck in another address by accident.

token_contract.approve(agent, tokens);

 }

This implementation could be used as is in DreamTeamStorage, but would require some tweaks if added in TeamContracts.

Conclusion

All the audited contracts are simple, straightforward and have an adequate amount of documentation to easily understand what they do. In particular, we haven’t found any important issues that we consider critical.