EthereumCommonwealth / Auditing

Ethereum Commonwealth Security Department conducted over 400 security audits since 2018. Not even a single contract that we audited was hacked. You can access our audit reports in the ISSUES of this repo. We are accepting new audit requests.
https://audits.callisto.network/
GNU General Public License v3.0
131 stars 34 forks source link

SportCrypt #216

Closed MillianoConti closed 5 years ago

MillianoConti commented 5 years ago

Audit request

A decentralized prediction market built on the ethereum blockchain. https://www.sportcrypt.com/trade/browse/all

Source code

https://github.com/sportcrypt/sportcrypt-contract/blob/master/SportCrypt.sol

Disclosure policy

https://t.me/joinchat/H9VK_xKZrN_WTlBTVowpCg

Platform

ETH

Number of lines:

381

MrCrambo commented 5 years ago

Auditing time 2 days

danbogd commented 5 years ago

Auditing time: 3 days

yuriy77k commented 5 years ago

@MrCrambo @danbogd assigned

danbogd commented 5 years ago

My report is finished.

RideSolo commented 5 years ago

Auditing time 3 days.

yuriy77k commented 5 years ago

@RideSolo assigned

yuriy77k commented 5 years ago

SportCrypt Security Audit Report

1. Summary

SportCrypt smart contract security audit report performed by Callisto Security Audit Department

2. In scope

3. Findings

In total, 2 issues were reported including:

No critical security issues were found.

3.1. It is necessary to check the input address to the zero-address.

Severity: low

Description

In the changeOwner and addAdmin functions, the input address is not checked for a null value.

Code snippet

https://github.com/sportcrypt/sportcrypt-contract/blob/e002324d630108f1be38d5b2f1685426fe0ab67f/SportCrypt.sol#L16 https://github.com/sportcrypt/sportcrypt-contract/blob/e002324d630108f1be38d5b2f1685426fe0ab67f/SportCrypt.sol#L20

3.2. Match ID

Severity: low

Description

Matches IDs are created offlines to avoid extra interactions with the contract and gas consumption, as described in the whitepaper:

"4.1 Match Creation: When new matches are offered for trading, they are added to our backend system which forwards them to all connected clients via websocket. The exchange doesn’t need to perform any on-chain actions to create a new match. This is a major scalability advantage of SportCrypt since it reduces the operational costs of the exchange and allows us to experiment with many simultaneous matches without worrying about ethereum gas overhead or waiting for blocks to be mined."

If a match id that is created by a third person (that is not related with the team ) is not picked by UI and finalized , this issues can represent a risk since someone can lure a user to use a match id that he created manipulating parameters like recoveryTimestamp and finalPrice.

A match can be finalized by an admin or after [recoveryTimestamp](https://github.com/RideSolo/sportcrypt-contract/blob/master/SportCrypt.sol#L251) has passed, if recoveryTimestamp is set low enough the finalPrice will be taken from the matchid and not set by the admin as it can be seen here.

Many situation can lead to an exploit of this issue, like creating a fake website but using the real contract address to show good activity of the contract transactions.

The described issue is not a direct exploit but require some social engineering to lure victims, developers can give more explanation to clear out this issue.

Code snippet

https://github.com/sportcrypt/sportcrypt-contract/blob/master/SportCrypt.sol#L254

https://github.com/sportcrypt/sportcrypt-contract/blob/master/SportCrypt.sol#L251

4. Conclusion

The audited smart contract can be deployed. Only low severity issues were found during the audit.

5. Revealing audit reports

https://gist.github.com/yuriy77k/6d0c21d54f9f03d36d8f897001af62ba

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

https://gist.github.com/yuriy77k/538e1f24c7367dacfa75d9a43d4ba0d1

MillianoConti commented 5 years ago

Reddit announcement

hoytech commented 5 years ago

Thank you very much for performing this audit on our code. We appreciate you taking the time and making your findings public.

Addressing the points in turn:

Zero-address checks

We acknowledge that checking for zero-addresses can eliminate a bug where an address parameter is not set at all, or is set to all 0s. In some cases it may make sense to put in a special check for this case. However, such checks won't help in the general case of sending an incorrect address. For this reason, code that passes addresses must be very careful that the addresses are valid.

The next version of our smart contract will not have owners or admins, so this particular instance will no longer be a concern.

Malicious Match IDs

This is a legitimate observation, but not something we are directly concerned about.

In order to interact with our smart contract (or any smart contract, really) a trusted program is required. In our case this program is our website dApp, which users trust because it is served over TLS (https) from our domain. If an attacker tricks a user into running a malicious version of our code or, equivalently, finds a XSS vulnerability on our website, then they could perform many malicious actions -- independent of whether or not they are able to create Match IDs accepted by the contract.

For example, a malicious version of the dApp could wait until a user initiates a trade on a valid Match ID, and then substitute the trade parameters with ones of its own choosing. For instance, the attacker could change the order to one of their own orders at a price of 1 or 99. The malicious dApp would then display fake results saying the trade went through at the desired price. From the perspective of metamask, the user would not be able to see any difference, at least until it was too late.

Note that this is a general property of dApps as they are currently implemented. Nearly all dApps rely on trusted code bootstrapped via a TLS (https) connection. When that trust is breached (because of fishing or an XSS bug) then the security of a system is jeopardized, regardless of any smart contract protections in place. A famous example of this was the 2017 EtherDelta XSS bug. In this case the attacker just went for the private keys stored by non-metamask users, but if they were more determined they could've done much more damage, including attacks on metamask users.

Since this behaviour is intended as part of our design, and we are aware of the risks, the next version of our smart contract will have a similar computation of Match IDs. However, we additionally plan to publish a separate version of our dApp that users can download, thereby reducing the risk of continuously relying on the TLS trust chain every time you want to use the site.