AdChain smart contract security audit report performed by Callisto Security Audit Department
The goal of the adChain Registry is to provide advertisers with a list of websites that offer high - quality inventory for serving digital ads.
https://publisher.adchain.com/domains
Commit hash: 5930cede2d533c8ee92001fd0be8eada2f2bc614
- Registry.sol
- historical/StandardToken.sol
- historical/Token.sol
- historical/HumanStandardToken.sol
- AttributeStore.sol
- Challenge.sol
- DLL.sol
- Migrations.sol
- Parameterizer.sol
- PLCRVoting.sol
- AttributeStore.sol
- historical/Disbursement.sol
- historical/Filter.sol
- historical/Migrations.sol
- historical/Sale.sol
In total, 11 issues were reported including:
-
2 medium severity issues.
-
8 low severity issues.
-
1 notes.
No critical security issues were found.
-
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 the following code to the transfer(_to address, ...) function:
require( _to != address(this) );
From ERC-20 specification:
The function SHOULD
throwif the_fromaccount balance does not have enough tokens to spend.
But in this implementation it just returns false. This can lead to serious consequences. Because checking the return value of this function is rare.
For example, external contract may use this token contract as:
AdChainToken.transferFrom(recipient, this, value);
points[recipient] += value;In this case recipient can get any value of points, but he may not have enough money and the code will succeed.
The function SHOULD
throwif the_fromaccount balance does not have enough tokens to spend.
It is possible to lose tokens by sending to 0x0 address.
Use condition like require(to != address(0)).
EIP20 says that:
Transfers of 0 values MUST be treated as normal transfers and fire the Transfer event. But in this contract, function
transferhas a condition:
if (balances[msg.sender] >= _value && _value > 0) {
// ...
}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.
An event isn't emitted when assigning the initial supply to the msg.sender.
The owners can 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.
There are two/three input arrays, but no check for the same length. In this case it is possible to skip some parameters.
isPassed function member of PLCRVoting contract is used by multiple function to define if a poll id that ended has passed or not.
A poll is a structure constituted of multiple parameters essential for the voting system success, like voteQuorum that is the minimum number of voter required to pass a poll.
However is in isPassed function the comparison used to validate if the poll has passed or not do not respect the minimum number of voters even if the quorum parameter is included, for example once the poll has ended and we have:
voteQuorum = 10
votesFor = 1
voteAgainst = 8
isPassed will return true since the result will be 100 * 1 > 10 * (1+8).
Developers should explain do use of such comparison since first voteQuorum is not respected and votesFor and voteAgainst are not compared fairly, resulting in a wrong vote result.
When a vote is committed using commitVote a _secretHash is saved into the voter parameters. The voter reveal the vote option using revealVote and inputting _voteOption and _salt that were used previously to compute the _secretHash and commit the vote.
Please note that when the user commit the vote he can use either _voteOption equal to 1 to vote for the _pollID or any other value to vote against, as we can see here.
However when the reward is distributed using claimReward member of Challenge library getNumPassingTokens function member of PLCRVoting contract is called, and the only values that will return the getNumTokens are if _voteOption was used with a value equal to 0 or 1. The consequences are if a user voted against using value different than zero his vote is counted but he won't get the reward even if the marjority voted against.
If we consider the token contract code is StandardToken.sol — it is ok. But considering that we are auditing not production version in the main network we have to assume that the contract of the token may be different. In this case it is theoretically possible the Reentrancy Attack.
For example when [calling] to double withdrawal because listing.unstakedDeposit updated after calling external contract(token.transfer).
In all the audited contracts the developers didn't use SafeMath to prevent over or underflow, all the code lines that might raise issues are listed below. Please note that this operation can overflow or underflow only is an error was made mainly by the owner.
Use SafeMath.
All calculations are carried out above the call of external contracts.
The audited smart contract should not be deployed. Reported issues must be fixed prior to the usage of this contract.
https://gist.github.com/yuriy77k/bd018ba219aaee55196e3fa486730c83
https://gist.github.com/yuriy77k/36a894ed52edb59ed72376ef96e049f0
https://gist.github.com/yuriy77k/29aff095d44e10cf7087a3e18a58d9d2