preliminary audit results

README.md

@@ -30,10 +30,11 @@ Also built in the token is a vesting schedule for limiting SNT transferability o
 Code for the SNT token and the offering is being reviewed by:
 
 - Jordi Baylina, Author.
-- [Smart Contract Solutions (OpenZeppelin)](https://smartcontractsolutions.com/). [Pending audit results](/)
-- [CoinFabrik](http://www.coinfabrik.com/). [Pending audit results](/)
+- [Smart Contract Solutions (OpenZeppelin)](https://smartcontractsolutions.com/). [Pending audit results](/) [Preliminary](/audits/prelim-smartcontractsolutions-ef163f1b6fd6fb0630a4b8c78d3b706f3fe1da33.md)
+- [CoinFabrik](http://www.coinfabrik.com/). [2152b17aa2ef584a2aea95533c707a345c6ccf69](/audits/coinfabrik-2152b17aa2ef584a2aea95533c707a345c6ccf69.pdf)
 - [BlockchainLabs](http://blockchainlabs.nz/). [Pending audit results](/)
 - [Bok Consulting](https://www.bokconsulting.com.au/). [Pending audit results](/)
+- [Holiman](https://github.com/holiman). [e6c70b8078aed6f9c04780c0b1e55dbe854dcf68](/audits/holiman-e6c70b8078aed6f9c04780c0b1e55dbe854dcf68.md) (Informal)
 - YYYYYY. [Pending audit results](/)
 
 A bug bounty for the SNT token and offering started on 14/6/2017. [More details](https://blog.status.im/status-network-token-bug-bounty-a66fc2324359)

audits/holiman-e6c70b8078aed6f9c04780c0b1e55dbe854dcf68.md

@@ -0,0 +1,91 @@
+These are some worknotes by Martin Holst Swende from looking into the Status.IM contracts, and does not constitute a full-scale audit. 
+
+Exclusions are
+
+- MiniMeToken.sol
+- MultisigWallet.sol
+
+Git commit: `e6c70b8078aed6f9c04780c0b1e55dbe854dcf68`. 
+
+# Notes
+
+## ContributionWallet
+
+Holds a ref to a `multisig`. The multisig can request funds after a certain block; `endBlock`, which is set on creation. 
+
+It will allow transfer if _either_ of these are true: 
+- The blocknumber is after `endBlock`
+- The `contribution` is `finalized`. 
+
+Potential improvements: 
+
+* Should check (upon creation) that `multisig` is a valid address
+* Should check (upon creation) that `endBlock` is reasonably within bounds
+* Should check (upon creation) that `contribution` is a valid address. 
+
+## Owned
+
+The `Owned` contract allows for setting `0x0` as owner. This may be dangerous. 
+The `Owned` contract does a single-step transfer of ownership. Another, more secure model, is a two-step handover: 
+
+1. `owner-> X.changeOwner(newOwner)`
+  * `_futureOwner = newOwner`
+2. `newOwner -> X.takeOwnership()`
+  * `owner = _futureOwner; _futureOwner = 0x0`
+
+
+## DevTokensHolder
+
+
+This code does two external calls (`balanceOf`):
+
+        uint256 balance = snt.balanceOf(address(this));
+        uint256 total = collectedTokens.add(snt.balanceOf(address(this)));
+
+Would save some gas if written as: 
+
+        uint256 balance = snt.balanceOf(address(this));
+        uint256 total = collectedTokens.add(balance);
+
+
+---
+
+This is a bit convoluted: 
+
+        if (finalized == 0) throw;
+        if (getTime().sub(finalized) <= months(6)) throw;
+
+    ...
+
+    function months(uint256 m) internal returns(uint256) {
+        return m.mul(30 days);
+    }
+
+    function getTime() internal returns(uint256) {
+        return now;
+    }
+
+Could be written more clearly as 
+
+	assert (  finalized > 0 && now > finalized + 6 months )
+
+
+
+---
+
+If tokens or ether are mistakenly sent to `DevTokensHolder`, they can be recovered via `claimTokens`. 
+
+The code casts the `_token` to a `MiniMeToken` - but the method can be used to extract any ERC20Token. In order to make this more clear, it would perhaps be better to include the `ERC20Token` interface, and do the cast to an `ERC20Token` instead.  
+
+Applies to a few other contracts aswell, e.g. `StatusContribution`. 
+
+
+## StatusContribution
+
+
+`proxyPayment` does not verify that `address _th` is actually a valid address (!= `0`)
+
+I'm not sure if having default function which buys tokens is a good idea; people may send money to if from contract wallets which are not capable of actually using the tokens afterwards. By forcing the use of a method, it's certain that the calling wallet can create arbitrary-data transactions. 
+
+
+

audits/prelim-smartcontractsolutions-ef163f1b6fd6fb0630a4b8c78d3b706f3fe1da33.md

@@ -0,0 +1,33 @@
+Hi all!
+
+Here are the severe issues we've found so far:
+
+#### Cloning a MiniMeToken with snapshot block set to the current block is subject to a value change attack.
+
+If a `MiniMeToken` is cloned by calling the `createCloneToken` function on [line 384 of MiniMeToken.sol](https://github.com/status-im/status-network-token/blob/2152b17aa2ef584a2aea95533c707a345c6ccf69/contracts/MiniMeToken.sol#L384) with the `_snapshotBlock` parameter set to zero or `block.number`, the clone will be creating using the current block as `parentSnapShotBlock`.
+
+This opens a small window of time for an attacker to see this transaction in the network, and insert `transfer` transactions in the same block. Given these will be [in the context of the current block too](https://github.com/status-im/status-network-token/blob/2152b17aa2ef584a2aea95533c707a345c6ccf69/contracts/MiniMeToken.sol#L500), the values used for the clone token will contain the modifications by the attacker. This would be confusing for the user creating the clone at the very least, and potentially dangerous. 
+
+Luckily this can be very easily fixed. Consider replacing [line 391 of MiniMeToken.sol](https://github.com/status-im/status-network-token/blob/2152b17aa2ef584a2aea95533c707a345c6ccf69/contracts/MiniMeToken.sol#L391) for a check that will throw if `_snapshotBlock` equals `block.number`. Consider adding the same check [in the MiniMeToken constructor](https://github.com/status-im/status-network-token/blob/2152b17aa2ef584a2aea95533c707a345c6ccf69/contracts/MiniMeToken.sol#L156) too, for `_parentSnapShotBlock`.
+
+#### ContributionWallet finalBlock can be different from StatusContribution stopBlock
+
+Withdrawal from the `ContributionWallet` contract is enabled once [the contribution is finalized](https://github.com/status-im/status-network-token/blob/2152b17aa2ef584a2aea95533c707a345c6ccf69/contracts/ContributionWallet.sol#L58) or when the current block is after `finalBlock`, a state variable. For the contract to serve its purpose, `finalBlock` should be the same as the contribution's `stopBlock` or greater. Otherwise, the funds could be withdrawn [before the contribution is finalized](https://github.com/status-im/status-network-token/blob/2152b17aa2ef584a2aea95533c707a345c6ccf69/contracts/ContributionWallet.sol#L57), which would defeat the purpose of `ContributionWallet`. 
+
+Consider using `contribution.stopBlock()` in place of `finalBlock` or checking that `finalBlock` is greater or equal than `stopBlock`.
+
+**Addendum**
+
+We also have and additional comment about [a PR you merged adding gas price cap to transactions](https://github.com/status-im/status-network-token/pull/20/files) and [a pending PR adding a cap to call frequency by address](https://github.com/status-im/status-network-token/pull/67/files). Both are out of the audit's commit scope, but we want to let you guys know our thoughts anyway as a friendly help. We don't this this additions are a good idea, and recommend you revert them. 
+
+See [Vitalik's post](http://vitalik.ca/general/2017/06/09/sales.html) about the gas price cap issue, and the frequency cap can be easily circumvented so it doesn't add security but does add complexity and attack surface.
+
+Let me know your thoughts/questions. The final report will be delivered on Friday, as agreed.
+
+Happy to be working on this together,
+
+Cheers,
+
+Manuel Araoz
+
+[openzeppelin.com](https://openzeppelin.com/)