This document is a security audit report performed by danbogd, where 2key network has been reviewed.
Сommit hash c85f3e1f3a04c56afd28bf673758367cc3df6609.
HIGH - vulnerability can be exploited at any time and cause a loss of customers funds or a complete breach of contract operability. (Example: Parity Multisig hack, a user has exploited a vulnerability and violated the operability of the whole system of smart-contracts (Parity Multisigs). This could performed regardless of external conditions at any time.)
MEDIUM - vulnerability can be exploited in some specific circumstances and cause a loss of customers funds or a breach of operability of smart-contract (or smart-contract system). (Example: ERC20 bug, a user can exploit a bug (or "undocumented opportunity") of transfer function and occasionally burn his tokens. A user can not violate someone else's funds or cause a complete breach of the whole contract operability. However, this leads to millions of dollars losses for Ethereum ecosystem and token developers.)
LOW - vulnerability can not cause a loss of customers funds or a breach of contracts operability. However it can cause any kind of problems or inconveniences. (Example: Permanent owners of multisig contracts, owners are permanent, thus if it will be necessary to remove a misbehaving "owner" from the owners list then it will require to redeploy the whole contract and transfer funds to a new one.)
In total, 30 issues were reported including:
-
5 high severity issues.
-
9 medium severity issues.
-
6 low severity issues.
-
6 notes.
-
4 owner privileges (the ability of an owner to manipulate contract, may be risky for investors).
Incoming addresses should be checked for an empty value(0x0 address) to avoid loss of funds or blocking some functionality. Code snippet
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/TwoKeyAirdropCampaign.sol#L88
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/token-pools/TokenPool.sol#L24-L25
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/token-pools/TwoKeyCommunityTokenPool.sol#L21-L23
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/token-pools/TwoKeyCommunityTokenPool.sol#L52
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/donation-campaign-contracts/TwoKeyDonationCampaign.sol#L38
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/donation-campaign-contracts/TwoKeyDonationConversionHandler.sol#L82
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/acquisition-campaign-contracts/TwoKeyAcquisitionCampaignERC20.sol#L45
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/singleton-contracts/TwoKeyEventSource.sol#L134
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/singleton-contracts/TwoKeySingletonesRegistry.sol#L55
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/upgradability/StructuredStorage.sol#L39
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/upgradability/UpgradabilityProxy.sol#L26
According to ERC20 standard, when initializing a token contract if any token value is set to any given address a transfer event should be emitted.
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/ERC20CustomToken.sol#L12
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/ERC20TokenMock.sol#L12
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/acquisition-campaign-contracts/TwoKeyAcquisitionCampaignERC20.sol#L78
Contract owner allow himself to:
[upgrade contract](https://github.com/2key/contracts/blob/295b13424a21ae9f52844a3a706af02aa4472fd4/contracts/2key/upgradability/UpgradeabilityProxy.sol#L26) and implement any logic in the new contract. And even if the new contract will be audited, at any time possible to change the address of the new contract again to not audited and insecure.
to [freeze](https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/singleton-contracts/TwoKeyEconomy.sol#L58) all transfers on ERC for some period of time
Owner can [change](https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/singleton-contracts/TwoKeyUpgradableExchange.sol#L400) any uint value in [TwoKeyUpgradableExchange](https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/singleton-contracts/TwoKeyUpgradableExchange.sol) contract as buy and sell rates, weiRaised values, etc.
Owner can [change](https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/upgradability/StructuredStorage.sol#L48) proxy logic contract any times and chan change it to not audited contract.
In function setInitialParams there is wrong value passing to function setInitialParameters(_erc20Address, TWO_KEY_SINGLETON_REGISTRY);, because in all other similar functions there is passing value from arguments of functions and also TWO_KEY_SINGLETON_REGISTRY variable not initialized in this contract.
There should be passed twoKeySingletonesRegistry instead of TWO_KEY_SINGLETON_REGISTRY.
setInitialParameters(_erc20Address, twoKeySingletonesRegistry);There are several input arrays, but no check for the same length. In this case it is possible to skip some parameters.
<<<<<<< HEAD
- https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/singleton-contracts/TwoKeyCongress.sol#L108-L109 ======= https://github.com/2key/contracts/blob/034b0b6d2713efda29f277c7e81329df9aabb473/contracts/2key/TwoKeyAirdropCampaign.sol#L88 https://github.com/2key/contracts/blob/839a291ef0c240448a079962b1bb153f71cb5919/contracts/2key/token-pools/TokenPool.sol#L24-L25 https://github.com/2key/contracts/blob/839a291ef0c240448a079962b1bb153f71cb5919/contracts/2key/token-pools/TwoKeyCommunityTokenPool.sol#L21-L23 https://github.com/2key/contracts/blob/839a291ef0c240448a079962b1bb153f71cb5919/contracts/2key/token-pools/TwoKeyCommunityTokenPool.sol#L52 https://github.com/2key/contracts/blob/295b13424a21ae9f52844a3a706af02aa4472fd4/contracts/2key/upgradability/UpgradeabilityProxy.sol#L26
b78ee16...
In function replaceMemberAddress there is setting old member info to new address in line 199, but memberAddress will still be old address, which should be replaced.
Create new Member structure for replaced address with copying memberSince, votingPower and name.
In function transferFrom there is adding conversionQuota to _to address balance, where conversionQuota is maximal ARC tokens that can be passed in transferFrom, but value will always be equal to 1. So in this function there will not be transferring tokens from one address to another, there will be burning from one and minting to another.
There is no checking that there will be enough tokens in line 344 in function buyTokensAndDistributeReferrerRewards as wrote in TODO comment.
//TODO: add require that there's enough tokens at this momentAdd checking that there is enough tokens.
In the function isCampaignEnded there is possibility that campaignRaisedAlready will be more than campaignHardCapWei so in this case this function will return false instead of true.
Check that campaignRaisedAlready is at least equal to campaignHardCapWei.
if(endCampaignWhenHardCapReached == true && campaignRaisedAlready >= campaignHardCapWei) {
return true;
}If statement in constructor has no code inside of it and could be deleted.
Using function transferFrom anyone can transfer other addresses voting points to himself. There is no checking for person who call this function, so he can pass his address as to address and transfer himself anyone's voting points.
Better to use transfer function so only voting points' owners will be able to transfer their voting points.
Function allowance show balanceOf functions result and doesn't show the amount of tokens that an owner allowed to a spender as wrote in comment above function.
In function changeTokenDistributionDate the shift value will be positive if _newDate less then tokenDistributionDate and negative otherwise. Therefore this value should be subtracted from tokenUnlockingDate instead of added.
Same issue occur in function changeDistributionDate in TwoKeyPurchasesHandler.sol contract
uint shift = tokenDistributionDate - _newDate;
// If the date is changed shifting all tokens unlocking dates for the difference
for(uint i=0; i<numberOfVestingPortions+1;i++) {
tokenUnlockingDate[i] = tokenUnlockingDate[i] + shift;
}Use subtraction shift instead of addition:
uint shift = tokenDistributionDate - _newDate;
// If the date is changed shifting all tokens unlocking dates for the difference
for(uint i=0; i<numberOfVestingPortions+1;i++) {
tokenUnlockingDate[i] = tokenUnlockingDate[i] - shift;
}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.14. an attacker can intentionally spam an array of users.
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.
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.
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 removing a member, there is no safe mathematical operation maxVotingPower-= votingPower. The following actions may cause an underflow:
addMember 0x1, maxVotingPower = 10
removeMember 0x1, maxVotingPower = 10
replaceMemberAddress 0x1 to 0x2, maxVotingPower = 10 (look at issue 3.17 where the removed user can restore his membership with his votingPower)
removeMember 0x2, maxVotingPower = 0-10 (underflow)
And the same scheme underflow 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.
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) );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.
If a value of factor is higher than 2 in powerLawRewards function the cumulative reward for every address (numberOfInfluencers) will be higher than the total allocated reward totalRewardEthWEI.
GetCode library function at access the code of another contract and load it into a bytes variable, this function is used in TwoKeyVoteToken to load contract code and calculate a hash that will be used to allow the contract or not to transfer tokens following the developers comments (check the code below).
function addContract(address _contractAddress) public onlyOwner {
require(_contractAddress != address(0), 'addContract zero');
bytes memory _contractCode = GetCode.at(_contractAddress);
bytes32 cc = keccak256(abi.encodePacked(_contractCode));
canEmit[cc] = true;
}
///@notice Modifier which will only allow allowed contracts to transfer tokens
function allowedContract() private view returns (bool) {
//just to use contract code instead of msg.sender address
bytes memory code = GetCode.at(msg.sender);
bytes32 cc = keccak256(abi.encodePacked(code));
return canEmit[cc];
return true;
}
modifier onlyAllowedContracts {
require(allowedContract(), 'onlyAllowedContracts');
_;
}However when the code is loaded no state variable that has changed after the deployment is taken into account meaning that an attacker can load the code of a pre approved contract, deploy a new contract with the admin address changed (an example can be if the contract set the admin address inside the constructor), this will allow the attacker to access the same privileges as the original allowed contract on TwoKeyVoteToken or any inherited contract.
Pease note that no function is implemented to remove the privileges for a contract
compare function member of TwoKeyCongress compare only the first 3 bytes of the allowed function signature, if a not allowed function contain the same 3 first bytes the proposal can pass the validation phase.
getAllMaintainers function implemented in of TwoKeyMaintainersRegistry and TwoKeyPlasmaMaintainersRegistry return an incomplete or a wrong array of maintainers addresses since it is only updated once when initialized but not updated when calling addMaintainer or removeMaintainer later on after initialization.
https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/singleton-contracts/TwoKeyMaintainersRegistry.sol#L104 https://github.com/2key/contracts/blob/c85f3e1f3a04c56afd28bf673758367cc3df6609/contracts/2key/singleton-contracts/TwoKeyPlasmaMaintainersRegistry.sol#L79