search

EthereumCommonwealth / Roadmap

GNU Lesser General Public License v2.1
57 stars 17 forks source link

Cold Staking contract bug bounty. #52

Closed Dexaran closed 5 years ago

Dexaran commented 5 years ago

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.

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 commented 5 years ago

https://gist.github.com/mgistrat/b7c36c536e61c0d256ec4bd42cb6ff98

Dexaran commented 5 years ago

Callisto Security Audit is finished.

https://github.com/EthereumCommonwealth/Auditing/issues/77

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

Dexaran commented 5 years ago

@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 commented 5 years ago

https://gist.github.com/kabachok2/634549b48cb5e0bb96c822e44b5e4666

yuriy77k commented 5 years ago

@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 commented 5 years ago

https://gist.github.com/betbether/187a40779c6ccb4c9931a789f78aa278

Dexaran commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

https://gist.github.com/mgistrat/2f111ba241e398f0476c717f923babe8

dieselc commented 5 years ago

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:

Issue:

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

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@yuriy77k my CLO address: 0x6652576D517388e0c71745aE1f388DdD78142202

Dexaran commented 5 years ago

@mgistrat paid https://explorer2.callisto.network/tx/0x469c47b6a81fcc6bfd7e0989745d2aecc38782a072068037e99dfd9e14a1c89c