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

Cold Staking contract bug bounty. #52

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

Comments

Projects
None yet
6 participants
@Dexaran
Copy link
Member

Dexaran commented Oct 11, 2018

Scope

  1. ColdStaking.sol.

Contract overview

This is a system contract of Callisto Network.
The main purpose of this contract is to allow users to stake their coins and receive interest on CLO emission as a reward for holding their coins. A user is staking coins by simply depositing it into the contract. The contract will receive 20% of block reward - this is enforced at protocol level. The user can not withdraw his deposit or staked coins before a certain period of time.

Contract provides specific functionality for Treasurer allowing him to (1) stop/unstop the contract, (2) withdraw the amount of funds allocated for staking rewards and (3) remove his Treasurer role privileges (not earlier than at block 1800000).

For more information read the formula description or staking implementation discussion.

Bug bounty

Rewards are paid in CLO. As of 11th October, 1 CLO = 0.00000221 BTC.

1. Critical issue. Up to 1,000,000 CLO (~2,2 BTC) reward for finding a critical bug.

A critical error is an error that can be directly exploited and cause a loss of funds for cold stakers regardless of circumstances.

2. Medium severity issue. 200,000 CLO (~0,442 BTC) for finding security vulnerabilities and bugs, that could not be directly exploited but can affect contracts in some specific circumstances and can cause a loss of funds for a certain stakers.

Any bugs that can occur in some specific circumstances and violate contracts workflow, resulting in a loss of funds for cold stakers.

3. Low severity issue. 50,000 CLO (~0,11 BTC) for finding security vulnerabilities and bugs, that can not affect users other than the sender of the transaction.

Any code flaw, that grants a user an opportunity to harm himself by causing a loss of funds for his staking account.

4. Minor observation, non-security issue. 10,000 CLO for valuable code improvements, non-security issues and other flaw reports.

Any code flaw, that can not cause a loss of funds or a direct breach of the contract but can cause inconveniences somehow.

Notes.

  • "loss of funds" means loss of deposited stake only. Any loss of "staking reward" will be classified as a medium severity issue.

  • comment improvements are not paid.

  • the cold staking contract is currently undergoing a security audit. Issues reported by security auditors also count. Security auditors do not receive bugbounty rewards since they are paid separate salaries.

  • please, do not reveal your bug reports before the end of security audit (it end date of the security audit will be announced at the comment below).

Participating

  1. Create a secret gist.

  2. Describe the bug in the created gist.

  3. Wait for security audit to end. Keep your gist private.

  4. Publish the link to your gist (URL) at the comment below.

The first person to create a bug-report gist will be rewarded. Reporting issues that were already reported will not be rewarded i.e. if two persons report the same issue, only the one who did it earlier, will be rewarded.

For any questions: dexaran@callisto.network

@mgistrat

This comment has been minimized.

@Dexaran

This comment has been minimized.

Copy link
Member

Dexaran commented Oct 14, 2018

Callisto Security Audit is finished.

EthereumCommonwealth/Auditing#77

All the bug reports may be published in the comments below now.

@Dexaran

This comment has been minimized.

Copy link
Member

Dexaran commented Oct 14, 2018

@mgistrat "A staker will lose the deserved reward if he makes an additional deposit after the end of the round in case of Timestamp less then now."

This is not a bug/security vulnerability.
This situation is described in "Staking Rules": https://callisto.network/blog/post/callisto-network-cold-staking-protocol/

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.

@kabachok2

This comment has been minimized.

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 14, 2018

@kabachok2

block.number == LastBlock, Always equal, so the condition will never be met. We will not be able to use smarkontrakt to destination.
Need to remove LastBlock initialization.

This is not a bug/security vulnerability.
uint public LastBlock = block.number; is one-time initialization during smart contract deploy.

@duychuongvn

This comment has been minimized.

@Dexaran

This comment has been minimized.

Copy link
Member

Dexaran commented Oct 16, 2018

@duychuongvn

Missing check user inactive.
If user inactive more than two years, this method still return reward when she claims

If a user has come to receive his reward, it means that he has become active.

Issue 2: Method function new_block() public
The method is not payable so how this statement work?

The new_block() function is called from other functions when a user attempts to perform any action with contract. It can be called from payable function.

@duychuongvn

This comment has been minimized.

Copy link

duychuongvn commented Oct 16, 2018

@Dexaran

If a user has come to receive his reward, it means that he has become active
It 's not make sense, there are 2 methods [ function claim() public only_stake] and [ function report_abuse(address _addr) public only_staker] with same permission.

The claim() method is used to claim stake, report_abuse() method is used to withdraw stake without any reward.
Why does user call report_abuse() method while they can call claim() to get reward first, after that they call withdraw_stake()? If she is inactive more than two years, she can claim reward and withdraw stake.
Then the workflow here doesn't work:

(5) When a staker does nothing for a certain amount of time (1 year) after the locking period has ended, then they are considered inactive and are removed from the staking contract. The inactive stake is returned to the stakers address. No reward shall be paid to inactive stakers.


The new_block() function is called from other functions when a user attempts to perform any action with contract. It can be called from payable function.

I know we call this method from other methods in this contract, why do we public this method?

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 16, 2018

@duychuongvn

Why does user call report_abuse() method while they can call claim() to get reward first, after that they call withdraw_stake()? If she is inactive more than two years, she can claim reward and withdraw stake.

report_abuse() is intended to return the deposit to an inactive user. It can be called by any staker who find an inactive user. Of cause, if user is active he can withdraw_stake() himself.

I know we call this method from other methods in this contract, why do we public this method?

It can be called if need to update StakingRewardPool, TotalStakingWeight, Timestamp and LastBlock for some reason.

@mgistrat

This comment has been minimized.

@dieselc

This comment has been minimized.

Copy link

dieselc commented Oct 18, 2018

https://gist.github.com/dieselc/68a7544b19f107275cd126230329f527

Issue Severity: Medium

Impact: Stakers reward divided by half or more (worst case scenario).

"Medium severity issue. 200,000 CLO (~0,442 BTC) for finding security vulnerabilities and bugs,
that could not be directly exploited but can affect contracts in some specific circumstances and
can cause a loss of funds for a certain stakers."

Audited contract commit hash: 244ed1d2c3fe39d3a65d9e901a2812c3364b7c28

Scenario:

  • Staker X stake at time T.
  • Staker X claim is made more than 27 days after the first start_staking call.

Issue:

if the claim is made in the worst case scenario 27+26 days after the first stake request:

  • _StakerWeight will represent only the first 27 days staker weight.
  • TotalStakingWeight will represent the 27+26 days total weight.
    Assuming a growth of 0% since the 27th day of total staked amount the real reward will be divided by half, any higher growth will reduce the reward more.

This issue is related with all stakers that wait more than the claim period.
Other scenarios can cause the same issue. the one presented is just one case.

Solution

Calculate the whole staking weight of a staker including the whole staking period.

@dieselc

This comment has been minimized.

Copy link

dieselc commented Oct 18, 2018

@Dexaran @yuriy77k, I didn't think about the total reward that will double but in case of block reward reduction it will not double. I will update the gist.

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 18, 2018

@dieselc By Cold Staking rules, staker will receive reward only for complete round interval (27 days).
If staker claim his reward after 27+26 days he receives the reward for 27 days, but he can do next claim after 1-day passed and will receive next reward for 27 days.
This is not a bug/security vulnerability.

@yuriy77k

This comment has been minimized.

Copy link
Member

yuriy77k commented Oct 18, 2018

@mgistrat

Function stake_reward may return the wrong value of the reward, if there were no other operations with the contract in this block. TotalStakingWeight will have old value.

As this function is used only to inform it cannot cause loss of money or incorrect behavior of the contract.
I can classify this as a minor observation, non-security issue (10,000 CLO).
Please, send your CLO address.

@mgistrat

This comment has been minimized.

Copy link

mgistrat commented Oct 19, 2018

@yuriy77k my CLO address: 0x6652576D517388e0c71745aE1f388DdD78142202

@Dexaran Dexaran added finished and removed in progress labels Dec 18, 2018

@Dexaran Dexaran closed this Dec 18, 2018

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