Skip to content
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

Lucky Strike v5 #288

Closed
luckystrikeico opened this issue Jun 13, 2019 · 10 comments

Comments

Projects
None yet
5 participants
@luckystrikeico
Copy link

commented Jun 13, 2019

Intro

Dear auditors!
I want to thank you for the previous audits. It was very useful.
Some of the noticed features like the following are clearly reflected on the ICO page in the project description.

  • Dividends distribution ( Tokens burning )
  • Owner Privileges ( Tokens distribution )
  • Jackpot Play Logic ( One bet - infinite chances concept )

If you find any discrepancy between the project description and the way how smart contract works - so it is a mistake.
Otherwise it is the auditor's observation and so player/investor can agree with it or not.

I want to ask the auditors to focus on the search of errors in the product but not on the reiteration of clearly performed peculiar features of the work.

There were 2 critical errors in the previous version of the smart contract missed by auditors:

1)It was an easily detectible bug in 'resetJackpotPlayIsRunning' function. 
We had: 
 jackpotPlayLastTimeStartedFromBlock[_jackpotType].sub(block.number) 
— which is obviously always throws an exception 
instead of:
block.number.sub(jackpotPlayLastTimeStartedFromBlock[_jackpotType])

2)In __callback function there was no handling for the case in which random value obtained from Oraclize did not pass verification.

It caused draw jackpots to be impossible and it’s critical

These errors could have been detected if the auditors had used Metamask, chosen Ropsten testnet and made several bets (how I asked in the previous audit inquiry).

And so let us focus on the search of errors. Thank you.

Finally I hope we will be able to provide Ethereum community with the reliable gambling smart-contract with unique gameplay.

Thank you.

Audit request

Lucky Strike, based fully in Ethereum smart-contract, is bringing the core philosophy of blockchain to the gambling industry – enhancing it with an ICO model we’re calling ‘Bet & Own.’

Source code

game contract

tokens contract

Disclosure policy

You can write about any issues directly in the comments.

Platform

ETH

Number of lines:

833 (1666 * 0.5 reaudit)

Previous audit

#253

Previous audit findings:

  • 3.1. Truncated Value (Invest & Play)
    A truncation can happen when computing the bought tokens, but we do not consider this as an issue.
    In the 'token sale period' tokens are like a bonus to players, to motivate them invest part of the bet to the game development. But 'investors' are always players.
    Thus we do not strive for absolute accuracy in the allocation of the amount between the game and investment.
    We could allow bets only in sizes that do not allow truncation during division, but we believe this unnecessarily complicates the smart contract and creates uncomfortable restrictions when playing

  • 3.2. Jackpot Play Logical Error
    This is not an error, but one of the main ideas in the project. Once played user will always participate in jackpot games.

  • 3.3. Unplayed Bet
    Fixed. Now from the unplayed bet we transfer proper part to jackpots, and the rest is returned to the user.

  • 3.4. Block Gas Limit
    The problem of gas limit resolved by setting maximum of ticket that can be purchased in one transaction.
    Its experimentally determined safe value is <= 333

  • 3.5. Owner Privileges
    No issue there. Done intentionally.

  • 3.6. Known vulnerabilities of ERC-20 token

So we just follow the ERC20 standard.
We choose not to allow web interface users to use 'approve' at all.
It still possible for users to call smart contract functions directly or using blockchain explorer, but we assume that such users are advanced users who understand the risks of using the smart contract bypassing the recommended web interface.

  • 3.7. Possibility of minting more than hardCap.

We know it's possible. But we don’t see a problem here.

Other changes

  • function resetJackpotPlayIsRunning changed
    (there was a bug in resetting jackpot status)

  • function getOraclizeCallbackAddress() added to be able to check incoming transactions from Oraclize (for testing and debugging)

  • function __callback was rewritten and event OraclizeRandomValueVerificationWasFiled was added,
    we also add OraclizeRandomValueVerificationWasFiledCounter variable, it will show numbers of failures
    this should help us to identify cases in which Oraclize returned random value verification was filed and to process this case properly (see code)

@MrCrambo

This comment has been minimized.

Copy link

commented Jun 16, 2019

Auditing time 3 days

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@MrCrambo assigned

@RideSolo

This comment has been minimized.

Copy link

commented Jun 17, 2019

Auditing time: 2 days.

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@RideSolo assigned

@MrCrambo

This comment has been minimized.

Copy link

commented Jun 19, 2019

My report is finished

@gorbunovperm

This comment has been minimized.

Copy link

commented Jun 20, 2019

Estimated auditing time is 2 days.

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@gorbunovperm assigned

@gorbunovperm

This comment has been minimized.

Copy link

commented Jun 25, 2019

My report is finished.

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

LuckyStrike V5 Security Audit Report

1. Summary

LuckyStrike V5 smart contract security audit report performed by Callisto Security Audit Department

Lucky Strike, based fully in Ethereum smart-contract, is bringing the core philosophy of blockchain to the gambling industry – enhancing it with an ICO model we’re calling ‘Bet & Own.’

https://lucky-strike.io/game/#/

2. In scope

  1. LuckyStrike
  2. LuckyStrikeTokens

3. Findings

In total, 5 issues were reported including:

  • 1 Critical severity issues.

  • 1 medium severity issues.

  • 1 low severity issues.

  • 1 notes.

  • 1 owner privileges (the ability of an owner to manipulate contract, may be risky for investors).

3.1. An attacker can block the contract

Severity: critical

Description

In current version the draw takes place by queue and each bet is played out one by one. In case of victory, the winner is paid a reward by transfer function. The peculiarity of this function is that in the case of throw on the recipient's side the entire transaction will be rollback. throw can be done intentionally by an attacker, if the recipient is another smart contract. Thus, the attacker can block the entire contract, making it impossible to place bets and draws.

Simple example of an Attackers contract:

contract Attacker {
    LuckyStrike public ls = LuckyStrike(address(0x1A77110391C07D3d67c8c55C6114A858cB45BB26));
    bool public blockMode = true;
    
    function turnBlockModeOn() public {
        blockMode = true;
    }
    
    function turnBlockModeOff() public {
        blockMode = false;
    }
    
    function () payable external {
        if(blockMode) {
            revert(); // LuckyStrike blocked;
        }
    }
    
    function bet() public payable {
        ls.placeABet.value(msg.value)();
    }  
}

Code snippet

3.2. Truncated Value (Invest & Play)

Severity: medium

Description

Following the answer to this issue, the tokens are said to be like a bonus for the players but the amount to be invested is subtracted from msg.value ("bet amount = msg.value - invested amount") when invest and play is called meaning that this issue is still applicable.

Please refer to the previous audit issue description to solve this error.

3.3. Sum Validation

Severity: note

Description

Inside allocateSum member of the game contract contain sumValidationPassed variable that is used to check if the allocated sum values are correct however no action is taken following the result of it.

Code snippet

        bool sumValidationPassed = false;
        
        if (
            (jackpotsSumAllocation[1] +
            jackpotsSumAllocation[2] +
            jackpotsSumAllocation[3] +
            jackpotsSumAllocation[4] +
            incomeSum +
            refSum +
            payToWinner) == _sum) {
            sumValidationPassed = true;
        }

3.4. Owner Privileges

Severity: owner privileges

Description:

  • adjustAllocation function allows the owner to reset the rates of the different jackpots and income rate as wished.
  • 70M tokens are first distributed by the owner that represent 10500 ether, the token sale hardcap is 4500 ether, meaning that the developers allow them self more than a third of the total income of the bet game, investors have to be aware of such usage.

Code snippet

3.5. Known vulnerabilities of ERC-20 token

Severity: low

Description

  1. It is possible to double withdrawal attack. More details here
  2. Lack of transaction handling mechanism issue. WARNING! This is a very common issue and it already caused millions of dollars losses for lots of token users! More details here

4. Conclusion

The audited smart contract must not be deployed. Reported issues must be fixed prior to the usage of this contract.

5. Revealing audit reports

https://gist.github.com/yuriy77k/47f3a778a7febfdbd10fafc3c654d8c7

https://gist.github.com/yuriy77k/e12af0290e90d575e1b59acac31c223f

https://gist.github.com/yuriy77k/46a92b0f3cd3edfb30c71ab57730ebc0

@luckystrikeico

This comment has been minimized.

Copy link
Author

commented Jul 4, 2019

@yuriy77k

  • 3.1. An attacker can block the contract – will be fixed
  • 3.2. Truncated Value (Invest & Play) – wontfix

Yes, we admit that value can be truncated but truncated value can be not more than value of 1 ticket or/and 1 token per 1 transaction.
It will be not fixed because if we return truncated part it will consume additional gas and need more computation in smart contract and it seems that real value of it will be very low for user.

Other possible solution is to allow user to place bets than allow division without truncation only. But it would be not good for gameplay.

@luckystrikeico luckystrikeico referenced this issue Jul 11, 2019

Open

Lucky Strike v6 #332

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.