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.)
Commit hash: c85f3e1f3a04c56afd28bf673758367cc3df6609
All source files from path https://github.com/2key/contracts/tree/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key
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.
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;
}
}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.
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.
- Only the contract itself can call the
removeMemberfunction. But the contract does not have this call, respectively, there is no way to remove the user. - The
removeMemberfunction 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.
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.
When removeing a member, there is no safe mathematical operation maxVotingPower-= votingPower. The following actions may cause an underflow:
addMember0x1,maxVotingPower= 10removeMember0x1,maxVotingPower= 10replaceMemberAddress0x1 to 0x2,maxVotingPower= 10 (look at issue 3.5 where the removed user can restore his membership with hisvotingPower)removeMember0x2,maxVotingPower= 0-10 (underflowing)
And the same scheme underflowing with minimumQuorum variable.
Solidity operates only with integers. Thus, if the division is done before the multiplication, the rounding errors can increase dramatically.
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.
Pairs of incoming arrays should be checked for the same length. Otherwise, an incorrect parameters may be assigned to a number of elements.
TwoKeyVoteToken contract is created but never used:
votingToken = new TwoKeyVoteToken(address(this));-
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
Add into a function transfer(address _to, ... ) following code:
require( _to != address(this) );Incoming addresses should be checked for an empty value(0x0 address) to avoid loss of funds or blocking some functionality.
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.
There are some dangerous vulnerabilities were discovered in these contracts.