As stated in the Solidity docs (http://solidity.readthedocs.io/en/develop/security-considerations.html#use-the-checks-effects-interactions-pattern), it is much safer to use the Checks-Effects-Interactions Pattern.

We can see 3 occurrences in the source code of MiniMeToken.sol where this pattern should be used to avoid improper situations :

• doTransfer (l.239) : the check overflow is done too late and if the condition is met, amount can be deduced from sender and not credited to receiver : the account balance is not respected.

• generateTokens (l.425) : totalSupplyHistory can be updated without balances to be : possible owner account balance value error.

• destroyTokens (l.442) : again totalHistory can be decremented without owner balance to be.

Each checks/throw should be done before doing any update of variable.

Be explicit to prevent spurious errors. Even including public.

In 'MiniMeToken.sol' the updateValueAtNow() function adds levels of complexity than can be avoided:

• calling push() is cheaper than increasing the checkpoint length and assigning value to the struct
• too many local variables are used

Proposed implementation:

    function updateValueAtNow(Checkpoint[] storage checkpoints, uint _value) internal  {
        if ((checkpoints.length == 0)
        || (checkpoints[checkpoints.length -1].fromBlock < getBlockNumber())) {
             checkpoints.push(Checkpoint({
                 fromBlock: uint128(getBlockNumber()),
                 value: uint128(_value)
             }));
        } else {
            checkpoints[checkpoints.length-1].value = uint128(_value);     
        }
    }

The issue is about b891a91 where it would introduce a major problem for normal buyers to time the crowdsale. Better not touch the curves at all for the SGT guaranteed buys and just overflow into a normal buy once they've filled their allocation.

Point block delay equal to difference between revealed point and previous if revealed after set block.

Use actual block number, not set, when calculating next.

Clarify that revealPoint should use less gas price than contribution TXs, to have previous TXs cleared before next point.

There have been substantial changes to the contracts since the audits took place. You can see the full diffs here for each audit relative to current master:
2152b17...master
ef163f1...master

Included in these changes are fairly significant changes to the contribution contract and the dynamic ceiling contract. These changes go far beyond minor bug fixes and addressing auditor concerns.

On your blog you have stated:

Independent audits suggest there are no issues that need resolving

Which I believe to be a bit disingenuous due to the volume of changes that have occurred since the audits took place.

The below has 108 instead of 1018 in the comment describing the percentage calculations:
https://github.com/status-im/status-network-token/blob/master/contracts/StatusContribution.sol#L270

The issue

Conflicting information about the minimum contribution. The smart contract source code must be changed to follow either of the below statements.

Your terms of condition states:

17. Minimum Contribution Amount:​ Contributions of less than 0.001 ETH do not result in theallocation of SNT, nor will they be refunded.

And the blog post FAQ states:

Q: What is the minimum amount of contribution? 0.01 ETH.

Resolution

Decide on the minimum contribution and align it across; the ToC, the FAQ, the actual smart contracts and their automated tests.

Severity

Likelihood of this the issue happening? Medium. I would expect a significant number of people wanting to contribute, but don't want to contribute more than the minimum amount.

Impact of the issue? Medium. The impact of someone following the stated ToC and otherwise doing a valid transaction but that will be rejected based on conflicting terms should be considered Medium I think.

Public variable equal to calculated blocks/24h set at deploy of DynamicCeiling.

Decide on if soft cap is one point? Otherwise add soft cap parameter to setHiddenPoints.

Options:

• All points except soft cap within blocks/24h, set after endBlock, modulo logic.
• All points except soft cap treated as being within blocks/24h, set block treated as offset from when soft cap limit is reached.
• Division logic of set blocks when allRevealed to fit within blocks/24h (deviates from spec).

We modified our SaleWallet for:

• Not suiciding it just in case.
• Allowing us to withdraw the funds if sale is finalized before final block (you want to have the money in the multisig ASAP).

https://github.com/aragon/aragon-network-token/blob/master/contracts/SaleWallet.sol

https://github.com/status-im/status-network-token/blob/master/migrations/2_deploy_contracts.js#L60-L64

The last parameter, collectMinimum is currently set to 10^12. This means that the min per-transaction buy-in is about $0.00035 USD. This is significantly less than the cost of gas required to buy-in, which means the long-tail is really long and effectively needlessly burning people's gas for no gain.

The min value on each curve point should be at least 1 attoeth higher than expected gas costs. Since we know the max gas price, it should be pretty easy to calculate the expected gas cost for a transaction and then set the minimum to be higher than that.

If cap is less than 0.01ETH do not use asymptotic cap logic, to prevent long tail spam attacks.

Prepend underscores to all event parameter.

https://github.com/status-im/status-network-token/blob/master/contracts/StatusContribution.sol#L184
"po" should be "to"

As I have expressed in the PR, I think not allowing multi-sig wallets is a bad idea. However, since you appear to be going forward with it you SHOULD tell the public about this decision in advance of the crowdsale. If you don't, there are likely to be many participants that end up burning gas in attempts to contribute with a multi-sig wallet as well as an increased support burden on Status.im, Parity, MetaMask, MEW and Mist teams as people go to them seeking assistance with why they can't contribute the crowdsale.

After confirmation, when we try to forward a call to another contract, then it gets executed but when I debugged, I found that the transaction is executed from user account and not from multisig contract.
Is this a correct behaviour of contract or am I doing something wrong?

Fix all spelling and grammar errors.

Make them easier to understand.

The way the contract is currently structured, there's no guarantee that the maximum amount of Ether raised will be under failsafe (e.g., no guarantee that they won't raise 500,000 ETH). This is because assert(totalNormalCollected.add(toCollect) <= failSafe); is only checked on buyNormal, not buyGuaranteed. There is a check within setGuaranteedAddress (require(_limit <= failSafe);), but that only check that each guaranteed address can't contribute more than failsafe, which isn't what a failsafe is meant to be.

There are two ways to fix this issue IMO. First, move that check to doBuy. This is the only option that truly guarantees a failsafe. This does mean that if a guaranteed buyer contributes too late, their tx won't go through. But then, they can submit with high gas price (since the check isn't in guaranteedBuy) and they can submit during the first ceiling (with only SGT holders being able to contribute to), so there's no reason why a guaranteed buyer can't get their transaction in within the first few blocks of the start of the sale.

A less preferred alternative would be, at a minimum, to keep track of the total amount of guaranteed limit allocated, and check that this doesn't exceed a certain amount. For instance,

function setGuaranteedAddress(address _th, uint256 _limit) public initialized onlyOwner { require(getBlockNumber() < startBlock); totalGuaranteedAllocated=totalGuaranteedAllocated.add(_limit); require(totalGuaranteedAllocated <= guaranteedFailSafe); guaranteedBuyersLimit[_th] = _limit; GuaranteedAddress(_th, _limit); }

Finally, the alternative is to recognize the current behavior as a feature and not a bug, and then stop advertising the failsafe (e.g., 300K ETH) as the maximum amount raised, which is currently what's being communicated.

Use a moving index logic that moves forward only when the limit has been reached.

Remove binary search.

Remove cap averaging to prevent long tail of txs.

Reduces gas cost and prevents spam attacks that tries to delay txs and catch the next limit combined with the current one.

• Verify curve logic with different parameters
• Time for whole crowdsale
• moveTo()
• Curve progression

This concerns many occurrences in source files MultiSigWallet.sol and MiniMeToken.sol.

There are several reason to use 'require(condition);' instead of 'if(!condition) throw;' :

1. Faster execution, consumes less gas

2. More clear : 'throw' naming is perturbing because it does not throw an exception that we can catch.

3. Solidity will certainly deprecate throw (ethereum/solidity#1793).

https://contribute.status.im/status-terms.pdf

Section 2.2 Token Pools

Pool D consisting of 29% of the SNT of Pool A ​, intended as a reserve for future
Contribution Periods will be created by the Smart Contract System at the end of the
Contribution Period. After the end of the Contribution Period, **Pool C** will be allocated
to a multisignature Wallet controlled by Status GmbH. Status GmbH is entitled to use
the SNT of Pool D to raise further funding for the Status Project...

Seems Pool C will be allocated should be Pool D...

    // Do not allow contracts to game the system
    require(!isContract(caller));

There’s a well‑known case where a caller has codesize of 0 and is a contract. It should always use a tx.origin comparison instead.
Still better to fix it for those who might copy this wrong code.

In:
https://github.com/status-im/status-network-token/blob/master/contracts/StatusContribution.sol#L163
in states that to remove a guaranteedAddress, you would set the _limit to 0.

However, at:
https://github.com/status-im/status-network-token/blob/master/contracts/StatusContribution.sol#L167
there is a require that will throw if _limit is 0.

Fix:
Either modify the require to allow _limit == 0, or change the relevant comment (depending on whether you wish to allow guaranteedAddresses to be removed).

What's the plan for deployment?

Migration scripts is a great way via Truffle.

Function calculateHash is defined in l166, yet revealCurve calls keccak256 by itself in l91.
This should be avoided as it is a source of broken and missmathcing behaviour. If for example further implementation decides to change the hash implementation from keccak256 to something else in the function but someone forgets changing the direct calls -> missmathcing behaviour

Will prevent spam attacks that delay txs for combined limits.

Refactor to reuse code.

Use unmodified MiniMeToken contract, if possible.

StatusContribution.setGuaranteedAddress(...) has a comment that the limit can be set to 0, but the require(_limit > 0 will throw.

SGT holders could be faced to an abnormal gas cost when collecting their corresponding SNT:

balance and account variables are set via a call to getValueAt(..., finalizedBlock).
Problem is that in the case of finalizedBlock is not the actual block, there will be a costly loop to perform a binary search in the checkpoints[] array.
Depending on the length of this array, the number of iterations will be at worst log2 (length)+1, i.e. for a length of 10000 it can take 15 iterations (https://en.wikipedia.org/wiki/Binary_search_algorithm).

A quick win could be to store these values at the end of the contribution period.

Another way can be to use mapping with block as the key instead of an array : no iteration at all is needed.

These optimizations can be done at the SGT.sol level.

Hi,

I'm running a tool that detects vulnerable contracts. While analyzing MiniMeToken, we found that function updateValueAtNow takes a uint256 and further cast it into a uint128. Some overflow attacks can be performed because of this:

in function generateTokens if curTotalSupply + _amount is larger than 2**128 but smaller than 2**256. The overflow check in line 422 can be bypassed. However, _value passed to updateValueAtNow is larger than 2**128, which will wrap around and start from 0.

A potential fix is to add one check before the value is stored(line 500 and 503):

require(_value < 2**128);

endBlock is much more clear. So that you don't accidentally assume that stopBlock is the last block. Makes it much more clear how the logic works.

https://github.com/status-im/status-network-token/blob/master/test/dynamicCeiling.js#L196 has a comma, which is a minor typo.