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 #100

Closed
luckystrikeico opened this issue Nov 9, 2018 · 18 comments

Comments

Projects
None yet
5 participants
@luckystrikeico
Copy link

commented Nov 9, 2018

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

https://gist.github.com/yuriy77k/2d80694c23b89c543e832715b0b89305 (Game contract)

https://gist.github.com/yuriy77k/8111757d30637066b3b4bdb60b3525d0 (Tokens contract)

Disclosure policy

You can write about any issues found in code directly in the comments. Thank you.

Platform

ETH

Complexity

Medium

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

@luckystrikeico We provide free audit for contract developer. Please provide web site of project with contact email on it.

@luckystrikeico

This comment has been minimized.

Copy link
Author

commented Nov 9, 2018

@yuriy77k The website is still in development stage, but I can confirm that I’m the contract developer by sending a sum of ETH from the address that is specified on etherscan as the contract creator to your address.

@luckystrikeico

This comment has been minimized.

Copy link
Author

commented Nov 11, 2018

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Nov 11, 2018

@luckystrikeico Thank you. Request approved.

@MrCrambo

This comment has been minimized.

Copy link

commented Nov 21, 2018

Auditing time 4 days

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

@MrCrambo assigned.

@luckystrikeico

This comment has been minimized.

Copy link
Author

commented Dec 6, 2018

@yuriy77k @MrCrambo can we pay to speed up the audit process?

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

@luckystrikeico you can pay for priority audit. Send 116270 CLO to 0x74682Fc32007aF0b6118F259cBe7bCCC21641600
After payment the audit will be completed within 3 days.

@RideSolo

This comment has been minimized.

Copy link

commented Jan 5, 2019

auditing time 3 days

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

@RideSolo assigned

@danbogd

This comment has been minimized.

Copy link

commented Jan 10, 2019

Auditing time: ~ 7 days.

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

@danbogd assigned

@luckystrikeico

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

Hi @yuriy77k,
I’ve just released a new version of the contract (the previous version doesn’t require an audit)
Game Contract
Tokens Contract
I would like to pay for the priority audit, could you please confirm the price and the wallet address. It would be easiest for me if I could pay in ETH or BTC.
Thank you.

@luckystrikeico

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

@yuriy77k I thought it would be better to close the this issue and create a new one

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

1. Summary

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

2. In scope

3. Findings

In total, 8 issues were reported including:

  • 2 medium severity issues.

  • 4 low severity issues.

  • 2 minor observation.

No critical security issues were found.

3.1. Fallback function

Severity: medium

Description

Users might not understand the sales functionality and deposit ether directly to contract like most ICO does and not get tokens. The token minting is done using LuckyStrike Contract address which means that if a user send ether by accident the team wouldn't be able to do anything to refund him since they don't have a direct access.

Code snippet

https://gist.github.com/yuriy77k/8111757d30637066b3b4bdb60b3525d0#file-luckystriketokens-sol-L359#L361

Recommendation

Only owner address should be allowed to deposit tokens (owner is set to be LuckyStrike).

3.2. Truncated Dividends Value

Severity: medium

Description

takeDividends function member of LuckyStrikeTokens contract compute the dividends value for a certain amount of token specified by the msg.sender, however the dividends value is truncated when the contract balance is first devided by the totalSupply.
for a correct computation the contract balance should be first multiplied by valueInTokens specified by the user then devided by the totalSupply.

Code snippet

https://gist.github.com/yuriy77k/8111757d30637066b3b4bdb60b3525d0#file-luckystriketokens-sol-L168

3.3. Owner Withdrawal

Severity: low

Description

withdrawAllByOwner function member of LuckyStrikeTokens is supposed to allow the owner to withdraw ether from the contract if totalSupply is null and the sales period has ended.
however following takeDividends function no ether is supposed to be left if totalSupply is null.

The only way that this function is useful is if ether is sent to the contract through the fallback function, and as highlighted before the fallback function should be removed.

Code snippet

https://gist.github.com/yuriy77k/8111757d30637066b3b4bdb60b3525d0#file-luckystriketokens-sol-L183#L189

3.4. Users Ether Loss (Invest & Play)

Severity: low

Description

If the amount of ether sent to the contract through invest, investAndPlay or placeABet is lower than ticketPriceInWei or not equal to the exact multiple of ticketPriceInWei the ether or part of it will be lost by the user since if its value is lower than ticketPriceInWei he won't get a ticket to play or a token as investment, msg.value is directly divided by ticketPriceInWei.

Code snippet

https://gist.github.com/yuriy77k/2d80694c23b89c543e832715b0b89305#file-luckystrike-sol-L1707

https://gist.github.com/yuriy77k/2d80694c23b89c543e832715b0b89305#file-luckystrike-sol-L1766

https://gist.github.com/yuriy77k/2d80694c23b89c543e832715b0b89305#file-luckystrike-sol-L1795

Recommendation

The remaining ether after each operations should be sent back to the user.

3.5. placeABet, Invest and InvestAndPlay

Severity: minor observation

Description

  • When using investAndPlay the fifth of the msg.value is added the he marketing fund, the bet value is set to be msg.value minus the fifth of it, however when minting msg.value is divided by ticketPriceInWei to get the tokens to be minted for that user knowing that 4/5 of msg.value was used for the bet only 1/5 should be used to compute the tokens amount to be minted not the whole msg.value.

  • When investing using invest the user get 75% tokens bonus.

  • When placeABet is used the whole amount is set for the bet.

Following the previous points each step allow different token bonuses and ticket number, contract developers should explain their intentions and the users should be informed to avoid confusion.

Code snippet

https://gist.github.com/yuriy77k/2d80694c23b89c543e832715b0b89305#file-luckystrike-sol-L1795#L1803

https://gist.github.com/yuriy77k/2d80694c23b89c543e832715b0b89305#file-luckystrike-sol-L1754#L1780

https://gist.github.com/yuriy77k/2d80694c23b89c543e832715b0b89305#file-luckystrike-sol-L1699#L1730

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

3.7. Zero address checking required

Severity: low

Description

No zero address checking in functions transferFrom, init

3.8. Wrong event names

Severity: minor observation

Description

Event names should start with uppercase letter, but it start with lowercase.
Lines 1415, 1454, etc.

4. Conclusion

The Audited contracts are not fully safe, the contracts developers should take the highlighted issues into consideration.

5. Revealing audit reports

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

https://gist.github.com/yuriy77k/764471730493e66ca9979d7c8b2a65bb

https://gist.github.com/yuriy77k/9a8422217622491e4f2c50b73abebe1a

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Hi @yuriy77k,
I’ve just released a new version of the contract (the previous version doesn’t require an audit)
Game Contract
Tokens Contract
I would like to pay for the priority audit, could you please confirm the price and the wallet address. It would be easiest for me if I could pay in ETH or BTC.
Thank you.

The payment must be in CLO. The priority fee is 120 000 CLO, please send it to 0x74682fc32007af0b6118f259cbe7bccc21641600
Give me know when paid.

@luckystrikeico

This comment has been minimized.

Copy link
Author

commented Feb 15, 2019

@yuriy77k

3.1. Fallback function

The ability to receive ETH from any address is left on purpose.
After ICO, we can increase the number of games (by adding new games contracts) without changing contract with tokens.
The situation that users will send ETH directly to smart contract with tokens seems very unlikely.

3.2. Truncated Dividends Value

As we make calculations in Wei, the possible remainder of division is so small that it can be neglected.

3.3. Owner Withdrawal

"no ether is supposed to be left if totalSupply is null" - No. Smart contract can still receive ETH, even if all tokens are burned.

3.4. Users Ether Loss

This is done deliberately to reduce the amount of gas consumed by this functions.
Required checks can be done on the frontend.

3.5. placeABet, Invest and InvestAndPlay

Changed: In new version of smart contract we have changed the logic of tokens minting.

3.6. Known vulnerabilities of ERC-20 token

It's well known vulnerability in approve function of ERC-20 standard.
ERC-20 recommends:

To prevent attack vectors like the one described here and discussed here, clients SHOULD make sure to create user interfaces in such a way that they set the allowance first to 0 before setting it to another value for the same spender.

We add this fuction in order to fully meet the standard, but our frontend does not implement 'approve' functionality. And we can assume that users working with a smart contract, bypassing our front-end, are advanced enough to know about the problem and the correct use of this function.

3.7. Zero address checking required

Very low severity. It's just a variant of using a wrong address.

3.8. Wrong event names

True, but does not affect functionality.

@yuriy77k

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

@luckystrikeico

3.1. Fallback function

The ability to receive ETH from any address is left on purpose.
After ICO, we can increase the number of games (by adding new games contracts) without changing contract with tokens.
The situation that users will send ETH directly to smart contract with tokens seems very unlikely.

For this purpose will be better to create a special function. For example: function Donate() public payable {}. Using the fallback function for this purpose is an issue. Recommend to fix it.

3.2. Truncated Dividends Value

As we make calculations in Wei, the possible remainder of division is so small that it can be neglected.

Its depends on totalSupply value. In common cases, totalSupply values have the same order as balance. Recommend to fix it.

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.