Pooledfund smart contracts audit

Source code version

The current audit was conducted against smart-contracts in the next repository:

[email protected]:pooledfund/contracts.git

with commit hash 6a0da7d77fdc17699255e501326d20073c7c6240

and last modification date: Fri Feb 23 08:30:03 2018 UTC

A list of smart contracts under audit:

SampleCampaign.sol
GenericCampaign.sol
GenericKYCCampaign.sol
MultiOwnable.sol
SafeMath.sol
ValidatorLib.sol
Util.sol
UpdatableFees.sol
UpdatableTarget.sol

Disclaimer

The audit does not give any warranties on the security of the code. One audit cannot be considered enough. We always recommend proceeding to several independent audits and a public bug bounty program to ensure the security of the smart contracts. Besides, a security audit is not an investment advice.

About the project

Pooledfund is a smart contract system, which allows many small ICO investors to gather their investments in one big investment fund and invest in some ICO as one big investor.

The core component is a GenericCampaign.sol contract, which implements all fund gathering, reward, bounty and token distribution logic. For each ICO, PooledFund team create special contract, which inherit GenericCampaign.sol, and implement buyTokens, claimTargetICORefund, claimTargetICOTokens functions in a custom manner.

Checked vulnerabilities

We have scanned Pooledfund Smart Contracts for commonly known and more specific vulnerabilities. Here are some of the commonly known vulnerabilities that we considered (the full list includes them but is not limited to them):

Reentrancy
Timestamp Dependence
Gas Limit and Loops
DoS with (Unexpected) Throw
DoS with Block Gas Limit
Transaction-Ordering Dependence
Use of tx.origin
Exception disorder
Gasless send
Balance equality
Byte array
Transfer forwards all gas
ERC20 API violation
Malicious libraries
Compiler version not fixed
Redundant fallback function
Send instead of transfer
Style guide violation
Unchecked external call
Unchecked math
Unsafe type inference
Implicit visibility level

Manual Analysis

Critical issues

Critical issues seriously endanger smart contracts security. We highly recommend fixing them.

GenericCampaign.sol:228:

function() public payable {}

Fallback function is redudant, because investors supposed to contribute with contribute function.

Moreover this fallback function, combined with GenericCampaign.sol:137:

// ALL or NOTHING: check if we receive some money back,
// we will not distribute it between contributors, throw instead;
assert(this.balance == expectedBalance);

leads to critical security vulnerability: everyone can jepardaize the campaign just by sending some ether to campaign contract, blocking all investors and owners from executing buyForEverybody function.

Medium severity issues

Medium issues can influence smart contracts operation in the current implementation. We highly recommend addressing them.

GenericCampaign.sol:141-149: Bounty transfer integrated into buyForEverybody functions. We highly recommend to remove this logic from buyForEverybody for the following reasons:
- Tokens can be freezed after purchase (balanceOf returns some amount, but transfer can throw an exception);
- claimBounty function already implements the functionality in more reliant "pulled transfer" manner;
- in claimBounty function logic is different (tokensWithdrawnByContributor is not updated in buyForEverybody);
- with bounty transfer, transaction gas cost increase (and potentially it can lead to DoS with Block Gas Limit).
GenericCampaign.sol:50: it will be impossible to collect fee less than 1%
MultiOwnable.sol:13:
```
event OwnerAdded(address indexed newOwner);
```
you will not be able to get new owner's address from a client if you make newOwner event field indexed.

Low severity issues

Low severity issues can influence smart contracts operation in future versions of code. We recommend to take them into account.

GenericCampaign.sol:50:233: buy() function has a misleading name. It seems like that this function related to buyForEverybody function and token acquisition process, but actually appears that it's related to contribution process.
GenericCampaign.sol:253: in spite of a limited gas amount for transfer, some reentrancy attacks are theoretically possible, we recommend to add some reentrancy defence.
GenericCampaign.sol:242: weiAmount = msg.value.sub(returnToSender); is the same as weiAmount = possibleContribution; (latter option is easier to understand)
SafeMath were copied from the OpenZeppelin repository. We suggested following the recommended way of using OpenZeppelin contracts (via the zeppelin-solidity NPM package). The approach ensures any bug fixes are easily integrated into the code base.
GenericCampaign.sol:327: it will be more optimal to calculate bounty, fee and availableTokens only once, during buyForEverybody
SampleCampaign.sol:18: The default function visibility level in Solidity is public. We recommend to explicitly define function visibility to prevent confusion.

Codestyle issues

Codestyle issues influence code conciseness and readability and in some cases may lead to bugs in future. We recommend to take them into account.

GenericCampaign.sol:75: bool private locked = false; - we reccomend to place it with other fields
address(this) - this expression doesn't make sense, because this is already an address
GenericCampaign.sol:131: unnecessary amount variable (weiRaised can be used instead and this option will be more readable)
We recommend using pure/view instead of constant modifier because latter considered obsolete.
- GenericCampaign:202
- GenericCampaign:211
- GenericCampaign:221
MultiOwnable.sol:29: owners[msg.sender] == true -- doesn't make sense, owners[msg.sender] already returns a bool
It is recommended to specify strict Solidity versions (e.g. pragma solidity 0.4.17;), as future compiler versions may handle certain language constructions in a way the developer did not foresee.
SampleCampaign.sol:61: We recommend removing all commented source code
There are many Solidity style guide violations. We recommend integrating Solidity linter into the project.

Tests

Smart contracts source code is covered by unit tests, however there is one failing test:

1 failing

  1) Contract: SampleCampaign should throw if bounty fee is too big:
     AssertionError: VM Exception must be returned: expected -1 to be above -1
      at Function.assert.isAbove (/usr/local/lib/node_modules/truffle/build/webpack:/~/chai/lib/chai/interface/assert.js:254:1)
      at _callee7$ (test/sample_campaign_deploy.js:129:16)
      at tryCatch (node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:65:40)
      at Generator.invoke [as _invoke] (node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:303:22)
      at Generator.prototype.(anonymous function) [as throw] (node_modules/babel-polyfill/node_modules/regenerator-runtime/runtime.js:117:21)
      at step (test/sample_campaign_deploy.js:5:191)
      at test/sample_campaign_deploy.js:5:402
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

We highly recommend to fix failing tests before going to production.

A source code of core contracts is covered by unit tests pretty well, but we recommend to cover source code by tests of GenericKYCCampaign.sol, SampleUpdatableCampaign.sol and Util.sol.

Full coverage report can be found below:

------------------------------|----------|----------|----------|----------|----------------|
 contracts/                   |    78.02 |    53.06 |    81.03 |     79.9 |                |
  Crowdsale.sol               |    85.71 |       50 |       70 |    86.36 |       21,32,51 |
  GenericCampaign.sol         |    92.08 |    60.42 |    95.83 |    92.59 |... 311,323,386 |
  GenericKYCCampaign.sol      |        0 |        0 |        0 |        0 |... 27,29,31,32 |
  MultiOwnable.sol            |      100 |       75 |      100 |      100 |                |
  SafeMath.sol                |      100 |       50 |      100 |      100 |                |
  SampleCampaign.sol          |      100 |      100 |      100 |      100 |                |
  SampleUpdatableCampaign.sol |        0 |      100 |       25 |        0 |... 55,56,60,61 |
  UpdatableFees.sol           |      100 |      100 |      100 |      100 |                |
  UpdatableTarget.sol         |       50 |       25 |       50 |       50 |          15,16 |
  Util.sol                    |        0 |        0 |        0 |        0 |... 15,17,19,21 |
  ValidatorLib.sol            |      100 |    83.33 |      100 |      100 |                |
 contracts/mocks/             |      100 |      100 |      100 |      100 |                |
  SampleCampaignMock.sol      |      100 |      100 |      100 |      100 |                |
------------------------------|----------|----------|----------|----------|----------------|
All files                     |    78.38 |    53.06 |    82.26 |     80.2 |                |
------------------------------|----------|----------|----------|----------|----------------|

Conclusion

In this report, we have considered the security of PooledFund Smart Contracts. High, medium, low severity and code style issues found in the code are described above.

frostiq/Pooledfund-audit.md