New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Callisto: Cold Staking #77

Closed
Dexaran opened this Issue Oct 9, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@Dexaran
Copy link
Member

Dexaran commented Oct 9, 2018

Audit request

Cold Staking smart-contract is the core smart-contract of Callisto Network that is intended to allow users to stake their CLO by locking it for a certain period of time.

The contract must allow users to:

  • deposit CLO into the contract after the contract launch date (i.e. "stake" it)
  • deposited CLO must be locked for the specified amount of time
  • withdraw CLO after the specified amount of time
  • withdraw staking rewards
  • throw out a staker if he is inactive for longer than 2 years

Implementation pattern:

  1. Security audit of the Cold Staking contract.
  2. Bug bounty.
  3. Contract will be compiled and deployed at Callisto Mainnet before the hardfork date.
  4. At 11th November, the hardfork №1 will be enabled. Contract will start to receive 20% of block rewards. Staking will not be enabled instantly.
  5. At 12th November 0:0 UTC the staking will become available.

Cold Staking values.

See EthereumCommonwealth/Roadmap#51 (comment)

Source code

https://github.com/EthereumCommonwealth/Cold-staking/tree/863846e510299b8cb07bab38c0b60d1bd78e9947

Disclosure policy

Publish everything.

Platform

CLO

Complexity

Low

@gorbunovperm

This comment has been minimized.

Copy link

gorbunovperm commented Oct 9, 2018

Estimated auditing time is 3 days.

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 9, 2018

@gorbunovperm assigned

@danbogd

This comment has been minimized.

Copy link

danbogd commented Oct 10, 2018

Estimated auditing time is 7 days.

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 10, 2018

@danbogd assigned

@pro100skm

This comment has been minimized.

Copy link

pro100skm commented Oct 10, 2018

Auditing time: 2 days

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 10, 2018

@pro100skm assigned

@gorbunovperm

This comment has been minimized.

Copy link

gorbunovperm commented Oct 11, 2018

My report is finished.

@danbogd

This comment has been minimized.

Copy link

danbogd commented Oct 12, 2018

My report is finished.

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 13, 2018

1. Summary

Callisto: Cold Staking security audit report performed by Callisto Security Audit Department

2. In scope

ColdStaking.sol

3. Findings

In total, 1 issues were reported including:

  • minor 1 observation.

No critical security issues were found.

3.1. Timestamp may have not right meaning. A round can go longer than 27 days.

Severity: minor observation.

Code snippet

Description

The round can go longer than 27 days in case of an increase block generation time to over 25 seconds for a long time.

In case when blocktime is more than 25 seconds the Timestamp will have not "timestamp of the last interaction" value (look at here). This will lead to a distortion of the flow of staking time.

Consider the problem by example:

Block id Block time, sec _seconds variable now Timestamp variable now - Timestamp = Recommendation
start value 1539260000 Mike make a stake.
1200000 35 25 1539260035 1539260025 10
1200001 35 25 1539260070 1539260050 20
1200002 35 25 1539260105 1539260075 30
... ... ... ... ... ... An hour has passed
1200103 35 25 1539263640 1539262600 1040 Passed 1 hour of real time, but the contract "thinks" that 43 minutes have passed.

I don't know whether a continuous change in the block generation time by a value greater than 25 seconds is possible. But if it is possible then the time inside the contract will differ from the real time. What will affect the reward.

4. Conclusion

No critical vulnerabilities were detected.

5. Revealing audit reports

https://gist.github.com/yuriy77k/0074bd128bb29b601702e080c95b1fa4

https://gist.github.com/yuriy77k/28b349a4f4fbf71639d6d35b6d4357a7

https://gist.github.com/yuriy77k/1c97d787d60faa6c2e82f1b30529bb51

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 13, 2018

@gorbunovperm

Notes regarding the https://gist.github.com/gorbunovperm/baec3a9b533b922c34a32a6a8d4f579f report.

A staker MUST NOT deposit funds into the staking contract during the locking period. Depositing funds during the locking period will restart the locking period and staking contract.

For example:

StakingRewardPool = 100
Alice's StakerWeight = 500
Bob's StakerWeight = 300
Carol's StakerWeight = 200
TotalStakingWeight = 500 + 300 + 200 = 1000

Lets Alice claim first. Her reward = 100 * 500 / 1000 = 50.
StakingRewardPool = 100 - 50 = 50
TotalStakingWeight = 1000 - 500 = 500

After Alice, Bob claim his reward = 50 * 300 / 500 = 30
StakingRewardPool = 50 - 30 = 20
TotalStakingWeight = 500 - 300 = 200


Lets change transactions order:
Bob claim first. His reward = 100 * 300 / 1000 = 30.
StakingRewardPool = 100 - 30 = 70
TotalStakingWeight = 1000 - 300 = 700

After Bob, Alice claim her reward = 70 * 500 / 700 = 50
StakingRewardPool = 70 - 50 = 20
TotalStakingWeight = 700 - 500 = 200

By keeping this variable we will be able to see when the staker performed actions with the contract for the last time, which can be useful in some cases. I know that it is possible to review transaction history, but watching the variable in the contract is easier.

Passed 1 hour of real time, but the contract "thinks" that 43 minutes have passed.

So when contract "thinks" that passed 1 hour, in real time may be passed more than 1 hour. Therefore a round can go longer by real time. But I do not think that such a situation is possible for a long time. Also, it will not lead to loss of the stakers reward.

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 13, 2018

@pro100skm

Notes regarding the https://gist.github.com/pro100skm/7316153c530f600090a1ac2fb1f79e53 report.

  • No wrong calculation. TotalStakingWeight is a sum of all stakers weight, not a single staker. And must to use TotalStakingAmount.
TotalStakingWeight += _seconds.mul(TotalStakingAmount);

@yuriy77k yuriy77k closed this Oct 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment