luckystrikeico commented 5 years ago

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

tokens

Disclosure policy

You can write about any issues directly in the comments.

Platform

ETH

Previous audit:

https://github.com/EthereumCommonwealth/Auditing/issues/288#issuecomment-508441569

Release notes:

[x] To prevent issue described in previous audit report (https://github.com/EthereumCommonwealth/Auditing/issues/288#issuecomment-507383615 : 3.1. An attacker can block the contract) we do not accept bets from other smart contracts, only from regular Ethereum addresses. See LuckyStrike.sol lines: 1724-1735, 1755-1758
Number of lines:

837 (1674 * 0.5 reaudit coefficient)

yuriy77k commented 5 years ago

@luckystrikeico We already did free audits for you, if you need more, you have to pay for each next audit. The fee for this audit is 837 USD.

luckystrikeico commented 5 years ago

Dear, @yuriy77k 1)Please check these diffs:

https://www.diffchecker.com/A0XELndI added 1 method, added 8 lines

https://www.diffchecker.com/ZcWUJWq6 no changes at all.

2)Why is it necessary to reaudit two contracts (1674 lines of code) one of which has not been changed at all and another has bugfix (8 lines of code) And both contracts have already had five audits.

Wouldn't it be more correct to reaudit only the changed part of the contract?

3)I paid for the audit twice. And the last critical findings were discovered because I clearly asked to check the gameplay. Now you offer to pay the full amount for checking bugfix (8 lines of code).

I do not insist on a free audit, but I want to understand the priorities of Callisto and understand the conditions.

Is your goal to do audits that the community and developers will trust or to do as many commercial audits for one project as possible?

Thank you

yuriy77k commented 5 years ago

Dear @luckystrikeico

1) You have added the isContract function and called it from theplaceABetInternal function. That 8 lines have no issues. That’s all I can say after seeing the differences. Does this audit satisfy you? 2) In order to provide you with a full report, we must review the entire contract in order to know how changes made will affect the operation of the contract as a whole. According to our rules, auditors should check all submitted files for auditing. I can't change these rules. If you want to reduce the cost of an audit, break the project into modules, and provide for audit only those modules that you need to check.

3) As you said, we have made five audits, but you paid only for two. So you got three audits for free. Please, note, we don't provide beta-testing or debugging service for developers until their product is safe. We are working for the community to show which contracts are secure and which aren't.

luckystrikeico commented 5 years ago

Dear @yuriy77k As you confirm that this bug https://github.com/EthereumCommonwealth/Auditing/issues/288#issuecomment-507383615 is fixed, then according to the previous audit there are no critical errors in the contract, right?

I hope you will not insist that this minor fix somehow affects the rest of the contract 🙂

My goal as a developer is simple: to get sure that there are no critical errors in the smart contract. So let’s keep things fair: The last fix does not affect the rest of the contract logic.

@Dexaran could you please confirm that you agree with that kind of audit policy? And I have to pay full price for reaudit (8 lines bugfix)

luckystrikeico commented 5 years ago

@yuriy77k @Dexaran Due to the fact that I have not received any answer about Callisto audit policy I would like to say it's simply dosn't fair to use 0.5 coefficient to re-audit 8 lines of code.

@Dexaran I hope you will revise Callisto audit policy in the nearest future.

@yuriy77k Please provide me your BTC-address and I will pay 837 USD and I hope that re-audit of 8 lines of code will not take 1 month. Thank you.

yuriy77k commented 5 years ago

Please provide me your BTC-address and I will pay 837 USD and I hope that re-audit of 8 lines of code will not take 1 month. Thank you.

@luckystrikeico Please, send it to 3QuFHJ6uBGMoHMwd749tAYQNJ25yVt85Hf address.

Dexaran commented 5 years ago

@luckystrikeico as @yuriy77k said already, we are not a debugging tool or repetitive testing organization.

I understand your concerns, but I would say that our main goal is to establish a decentralized organization which will serve as a third party expert for smart-contract code reviews. Our mission assumes that we will use the general formalized approach for auditing.

Even if in your particular case the changes did not affect the logic of the entire contract, we would not be able to use this approach for other smart contracts. In general, to confirm that the changes did not affect other parts of the contract automated tests are required.

I strongly recommend that you use automated tests to make sure that small fixes and minor changes do not affect other parts of your contract OR to show third party auditors that it is so. Also, I strongly recommend that you manually test your contract and make sure that everything works as it should and only then submit it for a professional audit.

I appreciate that you chose us for an audit and decided to use our platform. We appreciate your feedback, but it seems that you treat our goal a bit misrepresented. Unfortunately, we cannot use an individual approach for each separate project if we wish to build an autonomous semi-automated organization.

I would recommend that you use tools designed for this purpose for debugging and testing. Of course, you can use Callisto as your debugging framework, but it will be somewhat costly.

luckystrikeico commented 5 years ago

Please provide me your BTC-address and I will pay 837 USD and I hope that re-audit of 8 lines of code will not take 1 month. Thank you.

@luckystrikeico Please, send it to 3QuFHJ6uBGMoHMwd749tAYQNJ25yVt85Hf address.

@yuriy77k Please check.

luckystrikeico commented 5 years ago

@Dexaran, actually I thankful for your answer!

And I also appreciate your contribution in Ethereum development. I really like Callisto concept and that’s why I use it.

But I will permit myself to make 2 remarks and 1 question:

1) I think you are informed good enough how Truffle works with Oraclize 😉. Inspite of this fact we have developed the tool for the external testing of our contract. And every time before sending the contract for audit to Callisto we thoroughly test it.

2) If Callisto miss some mistakes and the contract consequently will be hacked so it will significantly reduce the value of audits and trust to Callisto. Therefore Callisto should make all efforts to find the errors including debugging and repetitive testing and generally all possible.

The developer and the community should know:”If smart contract was audited by Callisto it means this contract can be trusted” Isn’t it the main goal and value of Callisto?

The important question concerning the audit policy (I think it will be interesting not only for me but also for other developers):

Let's consider the following scenario:

Audit of the first version contract was completed by Callisto (issues founded)
The developer has made a request for the second version audit of the contract (with fixes)
Callisto auditors have found the new critical issue which was not found during the first audit and not related with fixes

If the issue found during second audit was either in first version of the contract ( but this issue was not discovered during first audit and the issue did not appear as a result of changes in the second version of the contract )

A)Does it mean that the first version audit was made not good enough?

B)And if it is in such a way, should the developer pay for the new audit of the previous one was made not good enough?

MrCrambo commented 5 years ago

Auditing time 4 days

yuriy77k commented 5 years ago

@MrCrambo assigned

RideSolo commented 5 years ago

Auditing time: 3 days.

yuriy77k commented 5 years ago

@RideSolo assigned

gorbunovperm commented 5 years ago

Estimated auditing time is 2 days.

gorbunovperm commented 5 years ago

My report is finished.

yuriy77k commented 5 years ago

@gorbunovperm assigned

MrCrambo commented 5 years ago

My report is finished

yuriy77k commented 5 years ago

LuckyStrike v.6 Security Audit Report

1. Summary

LuckyStrike v.6 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

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

From previous audit:

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 rollbacked. 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.

isContract function is not prevented this vulnerability because extcodesize(address) worked only for already deployed contracts. If an external function(placeABet()) is called from the constructor of an malicious contract then extcodesize(_attacker) is zero.

Simple example of an Attackers contract:

contract Attacker {
    LuckyStrike public ls = LuckyStrike(address(0xa819effd0a9c86619953a4b979079e9ba4dca3f2));
    bool public blockMode = true;

    constructor() public payable {
        ls.placeABet.value(msg.value)();
    }

    function turnBlockModeOn() public {
        blockMode = true;
    }

    function turnBlockModeOff() public {
        blockMode = false;
    }

    function () payable external {
        if(blockMode) {
            revert(); // LuckyStrike blocked;
        }
    }
}

Code snippet

LuckyStrike, lines 1762, 1995, 1923

Recommendation

Use require(msg.sender == tx.origin) to prevent smart contract calling.

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. No event call

Severity: low

Description

According to ERC20 standard when coins are minted(or burned) a Transfer event should be emitted. In function mint(address to, uint256 value, uint256 _invested) in line 339 in token contract there is no Transfer event call that funds transferred from zero address to to address.

         balanceOf[to] = balanceOf[to].add(value);

Also Transfer event has not been called in function init(address luckyStrikeContractAddress) in line 142 in token contract after setting balances of addresses in lines 159-162.

        balanceOf[0x7E6CdeE9104f0d93fdACd550304bF36542A95bfD] = 33040000;
        balanceOf[0x21F73Fc4557a396233C0786c7b4d0dDAc6237582] = 8260000;
        balanceOf[0x23a91B45A1Cc770E334D81B24352C1C06C4830F6] = 26600000;
        balanceOf[0x961f5a8B214beca13A0fdB0C1DD0F40Df52B8D55] = 2100000;

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/c4ab2c36433dd229e223149b296ed89f

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

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

yuriy77k commented 5 years ago

The comments about not exist issues.

Known vulnerabilities of ERC20 token. In the contract were implemented all recommendations to resolve those issues. The fact that this token implements ERC20 standard does not mean that it is subject to all the problems of this standard.
Block Gas Limit.. It's not an issue. If there is a limitation of maximum number of tickets in UI, but a user would like to cheat UI and buy more then limitation, his transaction will be throw.

luckystrikeico commented 5 years ago

@yuriy77k thank you!

EthereumCommonwealth / Auditing

Lucky Strike v6 #332

Audit request

Source code

Disclosure policy

Platform

Previous audit:

Release notes:

Number of lines:

LuckyStrike v.6 Security Audit Report

1. Summary

2. In scope

3. Findings

3.1. An attacker can block the contract

Severity: critical

Description

Code snippet

Recommendation

3.2. Truncated Value (Invest & Play)

Severity: medium

Description

3.3. Sum Validation

Severity: note

Description

Code snippet

3.4. Owner Privileges

Severity: owner privileges

Description:

Code snippet

3.5. No event call

Severity: low

Description

4. Conclusion

5. Revealing audit reports