EthereumCommonwealth / Cold-staking

BSD 4-Clause "Original" or "Old" License
21 stars 19 forks source link

State Variables Initialization #6

Closed RideSolo closed 6 years ago

RideSolo commented 6 years ago

The following variables are not initialized:

uint public LastBlock;
uint public Timestamp;

check here

The value of the following local variables will be affected.

uint _blocks = block.number - _LastBlock;
uint _seconds = now - Timestamp;

check here

It will also affect the future values of timestamp:

Timestamp += _seconds;

check here

If the above mentioned state variables won't be set in the constructor, the following condition will be true for a significant amount of time, which will stretch the time since the actual block time at the moment of writing is ~12 s, and users will be able to claim in less than the real 27 days:

 if (_seconds > _blocks * 25) 
{
    _seconds = _blocks * 25;
}

If the state variables are initialized in the constructor, the above mentioned condition won't be required. and Timestamp can be set directly with now value

here is what I think can optimize the code and solve the issue: https://github.com/EthereumCommonwealth/Cold-staking/compare/master...RideSolo:master

yuriy77k commented 6 years ago

This code used for protection from manipulations with block.timestamp. To resolve this issue need to add constructor:

constructor () public {
        Timestamp = now;
    }
RideSolo commented 6 years ago

If block timestamp is set too far in the future it will be rejected probably (higher than 30 sec from the last block will probably be rejected). If the contract function can tolerate a X-second drift in time, it is safe to use block.timestamp. X can be determined following the average block time where X can be the double of the block time. therefore the code isn't really necessary.

yuriy77k commented 6 years ago

If block timestamp is set to far in the future it will be rejected probably (higher than 30 sec from the last block will probably be rejected). If the contract function can tolerate a X-second drift in time, it is safe to use block.timestamp. X can be determined following the average block time where X can be the double of the block time. therefore the code isn't really necessary.

I agree with you, but I added that code at the Dexaran's request.

RideSolo commented 6 years ago

I agree with you, but I added that code at the Dexaran's request.

No problem, maybe he can explain us the reason.

Dexaran commented 6 years ago

@RideSolo

If block timestamp is set too far in the future it will be rejected probably (higher than 30 sec from the last block will probably be rejected).

It is not the case for a large pool. A large pool can manipulate the timestamp and transactions so that if it finds a block, it can withdraw staking reward in more favorable conditions. For example, it can access The First Stake rewards in this manner.