2key network

ETH_2keyNetwork_report.md

2key network security audit report

Summary

This is the report from a security audit performed on 2key network by gorbunovperm.

Smart contracts allowing Social Sourcing - Incentivised Activation of Result-Driven online virality. More can be found on 2key.network (litepaper, whitepaper etc.)

In scope

Commit hash: c85f3e1f3a04c56afd28bf673758367cc3df6609

All source files from path https://github.com/2key/contracts/tree/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key

Findings

In total, 13 issues were reported including:

• 1 critical severity issue.

• 2 high severity issue.

• 3 medium severity issues.

• 7 low severity issues.

• 0 owner privileges.

• 0 note.

Security issues

3.1. Converter can block the rejection of himself

Severity: medium

Description

Converter can block the rejection of himself and continue the conversions that allowed if converter is not REJECTED.

During the rejection process, the funds are returned to the Converter via transfer function. But the recipient may reject the acceptance of funds and, accordingly, the entire transaction. This can be done with a simple smart contract:

pragma solidity ^0.4.24;

contract Attacker {
  bool public reject = true;
  function () payable public {
    if (reject)
      throw;
  }
}

Code snippet

• donation-campaign-contracts/TwoKeyDonationConversionHandler.sol

• donation-campaign-contracts/TwoKeyDonationCampaign.sol

3.2. Not complete removal of a user from Congress

Severity: low

Description

There are no restrictions for adding same user several times. In this case during the user removing only first record will be removed from allMembers array. The rest of the user records will be saved.

Code snippet

• addMember()

3.3. Failure to delete a user is possible

Severity: high

Description

It may happen that the user cannot be removed in case if block gas limit is reached. When removed, two cycles are used and when a large number of users will not have enough gas to perform the function. And considering vulnerability 3.2. an attacker can intentionally spam an array of users.

Code snippet

• removeMember()

3.4. There is no way to remove user

Severity: high

Description

1. Only the contract itself can call the removeMember function. But the contract does not have this call, respectively, there is no way to remove the user.
2. The removeMember function contains this code:

    for (uint j = i; j< allMembers.length; j++){
        allMembers[j] = allMembers[j+1];
    }

But the last assignment will not be performed. Because if the array consists of 5 elements then allMembers[4] = allMembers[5] expression attempts to access a nonexistent element(last index is 4). And transaction will be reverted.

Note: Also access to nonexistent element is performed here.

Code snippet

1. msg.sender == address(this)
2. a non-existent element

3.5. Anyone can get access to onlyMember functions

Severity: critical

Description

Anyone can call replaceMemberAddress function and there is no check of membership. Therefore, an attacker can add any address to isMemberInCongress array and get access to all functions with onlyMember modifier.

Code snippet

• isMemberInCongress = true

3.6. Underflowing is possible

Severity: medium

Description

When removeing a member, there is no safe mathematical operation maxVotingPower-= votingPower. The following actions may cause an underflow:

1. addMember 0x1, maxVotingPower = 10
2. removeMember 0x1, maxVotingPower = 10
3. replaceMemberAddress 0x1 to 0x2, maxVotingPower = 10 (look at issue 3.5 where the removed user can restore his membership with his votingPower)
4. removeMember 0x2, maxVotingPower = 0-10 (underflowing)

And the same scheme underflowing with minimumQuorum variable.

Code snippet

• maxVotingPower underflowing
• minimumQuorum underflowing

3.7. Multiplication after division

Severity: low

Description

Solidity operates only with integers. Thus, if the division is done before the multiplication, the rounding errors can increase dramatically.

Code snippet

• Multiplication after division

3.8. Adding a mamber is possible only from constructor

Severity: low

Description

The addMember function is used condition:

  if(initialized == true) {
      require(msg.sender == address(this));
  }

But after the initialization there is no way to use addMember because it is not being called from anywhere else in the contract.

Code snippet

• Condition

3.9. Check length of incoming arrays

Severity: low

Description

Pairs of incoming arrays should be checked for the same length. Otherwise, an incorrect parameters may be assigned to a number of elements.

Code snippet

1. initialMemberTypes
2. initialMembers

3.10. Unused contract

Severity: low

Description

TwoKeyVoteToken contract is created but never used:

  votingToken = new TwoKeyVoteToken(address(this));

Code snippet

• new TwoKeyVoteToken

3.11. Known vulnerabilities of ERC-20 token

Severity: low

Description

• It is possible to double withdrawal attack. More details here

• Lack of transaction handling mechanism issue. WARNING! This is a very common issue and it already caused millions of dollars losses for lots of token users! More details here

Code snippet

• InvoiceTokenERC20.sol
• TwoKeyEconomy.sol
• TwoKeyCampaign.sol

Recommendation

Add into a function transfer(address _to, ... ) following code:

require( _to != address(this) );

3.12. Checking input addresses

Severity: low

Description

Incoming addresses should be checked for an empty value(0x0 address) to avoid loss of funds or blocking some functionality.

Code snippet

• owner = _tokensOwner

3.13. Out of Gas

Severity: medium

Description

The contracts have loops the number of iterations of which depends on the variables in the storage and can increase over time. This can lead to "Out of Gas" error or to "Block Gas Limit". Need to create a mechanism for breaking cycles into parts.

Code snippet

1. moveFromStateAToStateB

2. setVisitsListTimestamps

3. moveFromStateAToStateB

4. visited

Conclusion

There are some dangerous vulnerabilities were discovered in these contracts.