Thales Binary Option Market Smart Contract Audit
# 1. Introduction
iosiro was commissioned by Thales DAO to conduct a smart contract audit of the Thales Binary Options Market system. The audit was performed between 9 June 2021 and 14 June 2021, consuming a total of 8 resource days.
This report is organized into the following sections.
* **[Section 2 - Executive summary:](#section-2)** A high-level description of the findings of the audit.
* **[Section 3 - Audit details:](#section-3)** A description of the scope and methodology of the audit.
* **[Section 4 - Design specification:](#section-4)** An outline of the intended functionality of the smart contracts.
* **[Section 5 - Detailed findings:](#section-5)** Detailed descriptions of the findings of the audit.
The information in this report should be used to better understand the risk exposure of the smart contracts, and as a guide to improving the security posture of the smart contracts by remediating issues identified. The results of this audit are only a reflection of the source code reviewed at the time of the audit and of the source code that was determined to be in-scope.
The purpose of this audit was to achieve the following:
* Identify potential security flaws.
* Ensure that the smart contracts functioned according to the documentation provided.
Assessing the off-chain functionality associated with the contracts, for example, backend web application code, was out of scope of this audit.
Due to the unregulated nature and ease of transfer of cryptocurrencies, operations that store or interact with these assets are considered very high risk with regards to cyber attacks. As such, the highest level of security should be observed when interacting with these assets. This requires a forward-thinking approach, which takes into account the new and experimental nature of blockchain technologies. Strategies that should be used to encourage secure code development include:
* Security should be integrated into the development lifecycle and the level of perceived security should not be limited to a single code audit.
* Defensive programming should be employed to account for unforeseen circumstances.
* Current best practices should be followed where possible.
<a name="section-2"></a>
# 2. Executive summary
This report presents the findings of an audit performed by iosiro on the Thales Binary Options Market system.
#### Audit findings
No findings remained open at the conclusion of the audit.
During the audit, a high-risk issue was reported to the Thales team, which would have prevented all users from exercising their options under specific circumstances. A medium-risk issue relating to insufficient access controls was also raised and subsequently remediated. The team also implemented a number of general code hygiene recommendations and addressed all informational items reported.
#### Recommendations
At a high level, the security posture of the system was found to be robust. In the long term it could be further improved by the following actions:
* Performing additional audits at regular intervals, as security best practices, tools, and knowledge change over time. Additional audits over the course of the project's lifespan ensure the longevity of the codebase.
* Creating a bug bounty program to encourage the responsible disclosure of security vulnerabilities in the system.
<a name="section-3"></a>
# 3. Audit details
## 3.1 Scope
The source code considered in-scope for the assessment is described below. Code from all other files was considered to be out-of-scope. Out-of-scope code that interacts with in-scope code was assumed to function as intended and not introduce any functional or security vulnerabilities.
### 3.1.1 Smart contracts
* **Project name: contracts**
* **Commits:** [8588175](https://github.com/thales-markets/contracts/tree/8588175b2d193005dfdb7b8aa3f70e4cbde6a05f)
* **Final Commit:** [c1de7b2](https://github.com/thales-markets/contracts/tree/c1de7b20ae66f5ea2608af7abe83d329308ed1e6)
* **Files:** BinaryOption.sol, BinaryOptionMarket.sol, BinaryOptionMarketData.sol, BinaryOptionMarketFactory.sol, BinaryOptionMarketManager.sol, BinaryOptionMarketMastercopy.sol, BinaryOptionMastercopy.sol, OwnedWithInit.sol
## 3.2 Methodology
A variety of techniques, described below, were used to conduct the audit.
### 3.2.1 Code review
The source code was manually inspected to identify potential security flaws. Code review is a useful approach for detecting security flaws, discrepancies between the specification and implementation, design improvements, and high risk areas of the system.
### 3.2.2 Dynamic analysis
The contracts were compiled, deployed, and tested in a test environment, both manually and through the test suite provided. Manual analysis was used to confirm that the code was functional and to discover whether any potential security issues identified could be exploited. The coverage report of the provided tests as on the final day of the audit is given below.
File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines |
---------------------------------------|----------|----------|----------|----------|----------------|
BinaryOption.sol | 100 | 87.5 | 100 | 97.44 | 72 |
BinaryOptionMarket.sol | 100 | 91.67 | 96.55 | 99.08 | 185 |
BinaryOptionMarketFactory.sol | 100 | 100 | 100 | 100 | |
BinaryOptionMarketManager.sol | 100 | 95.83 | 100 | 100 | |
contracts/interfaces/ | 100 | 100 | 100 | 100 | |
IBinaryOption.sol | 100 | 100 | 100 | 100 | |
IBinaryOptionMarket.sol | 100 | 100 | 100 | 100 | |
IBinaryOptionMarketManager.sol | 100 | 100 | 100 | 100 | |
All files | 100 | 93.14 | 98.63 | 99.25 | |
Overall, the unit and integration tests were found to be expansive and of high quality.
### 3.2.3 Automated analysis
Tools were used to automatically detect the presence of several types of security vulnerabilities, including reentrancy, timestamp dependency bugs, and transaction-ordering dependency bugs. The static analysis results were manually reviewed and any false positives were removed from the results. Any true positive results were included in this report.
Static analysis tools commonly used include Slither, Securify, and MythX. Tools such as the Remix IDE, compilation output, and linters could also be used to identify potential areas of concern.
## 3.3 Risk ratings
Each issue identified during the audit has been assigned a risk rating. The rating is determined based on the criteria outlined below.
* **High risk** - The issue could result in a loss of funds for the contract owner or system users.
* **Medium risk** - The issue resulted in the code specification being implemented incorrectly.
* **Low risk** - A best practice or design issue that could affect the security of the contract.
* **Informational** - A lapse in best practice or a suboptimal design pattern that has a minimal risk of affecting the security of the contract.
* **Closed** - The issue was identified during the audit and has since been addressed to a satisfactory level to remove the risk that it posed.
<a name="section-4"></a>
# 4. Design specification
The following section outlines the intended functionality of the system at a high level. The specification is based on the implementation in the codebase and any perceived points of conflict should be highlighted with the auditing team to determine the source of the discrepancy.
## Market dynamics
Any user could create a new `BinaryOptionMarket` instance using the `BinaryOptionMarketFactory`, as long as they were able to provide the initial minimum deposit. When creating the market, the user specified a strike price and the maturation date of the market. Upon successful creation, equal amounts of Short and Long options were minted to represent the creator's initial deposit.
Other users wishing to partake in the market could mint new pairs of `BinaryOption` tokens (both Long and Short positions). Unlike the initial market creator, these users were charged a small fee for minting. This fee was distributed to the Thales Markets' fee pool and market creator. The minted tokens could then be sold or bought on any compatible third-party exchange or transferred, as the tokens adhered to the ERC-20 token standard.
Any user could resolve the market after its maturation date, locking the exercise price of the market. Thereafter, users were able to exercise their options, burning their tokens in the process. If the price of the asset was above the strike price, users exercising their Long `BinaryOption` tokens were rewarded. Conversely, holders of Short options were rewarded if the price was below the strike price.
Markets expired 6 months after the maturation date. The owner of the `BinaryOptionMarketManager` contract was able to destroy the expired market and extract any remaining assets. Consequently, users are incentivized to exercise their options as soon as possible after market maturation.
## Upgradeability
The `BinaryOptionMarket` and `BinaryOption` token contracts made use of a minimal proxy to minimize the gas costs associated with creating a new market. While none of the contracts made use of an upgradeable proxy pattern, functionality to migrate markets between `BinaryOptionMarketManagers` was implemented.
## Oracles, pricing and supported assets
The smart contracts made use of Synthetix's `ExchangeRates` proxy to determine the direction of the market at maturation. All markets were dominated in sUSD, with contract logic supporting any base Synths (inverse Synths are disallowed). However, not all Synths were automatically enabled, as assets needed to be added via the `BinaryOptionMarketManager`.
## Ownership
The `BinaryOptionMarketManager` made use of a `onlyOwner` access modifier for most of its functions. This owner address was set during the contract's deployment and was expected that it would be set to the Thales Markets' governance contract address.
<a name="section-5"></a>
# 5. Detailed findings
The following section details the findings of the audit.
## 5.1 High risk
No high-risk issues were present at the conclusion of the review.
## 5.2 Medium risk
No medium-risk issues were present at the conclusion of the review.
## 5.3 Low risk
No low-risk issues were present at the conclusion of the review.
## 5.4 Informational
No informational issues were present at the conclusion of the review.
## 5.5 Closed
### 5.5.1 Rounding of fees can prevent all options from being exercised (high risk)
*[BinaryOptionMarket.sol#L282](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarket.sol#L282)*
#### Description
The manner in which the fees were accounted for during minting and the total amount of fees transferred when resolving a market could result in insufficient funds to pay out all options when exercised.
The issue could be reproduced using the following test case:
<pre>
<code class="language-javascript" >
var creatorFee = toBN("80839200000000000");
var poolFee = toBN("18178000000000000");
var value = toBN("3982999999999999700");
await manager.setPoolFee(poolFee, { from: managerOwner });
await manager.setCreatorFee(creatorFee, { from: managerOwner })
let now = await currentTime();
const result = await manager.createMarket(
sAUDKey,
initialStrikePrice,
now + timeToMaturity,
toUnit(2),
{
from: initialCreator,
}
);
market = await BinaryOptionMarket.at(
getEventByName({ tx: result, name: 'MarketCreated' }).args.market
);
await market.mint(value, { from: exerciser });
await fastForward(timeToMaturity + 100);
await exchangeRates.updateRates(
[sAUDKey],
[(await market.oracleDetails()).strikePrice],
await currentTime(),
{ from: oracle }
);
await market.exerciseOptions({ from: initialCreator });
await market.exerciseOptions({ from: exerciser });
await manager.expireMarkets([market.address], { from: managerOwner });
</code>
</pre>
The stack trace of the test case is given below for reference:
<pre>
<code class="language-javascript">
Error: VM Exception while processing transaction: revert SafeMath: subtraction overflow
at BinaryOptionMarketMastercopy.sub (openzeppelin-solidity-2.3.0/contracts/math/SafeMath.sol:43)
at BinaryOptionMarketMastercopy._decrementDeposited (contracts/BinaryOptionMarket.sol:223)
at BinaryOptionMarketMastercopy.exerciseOptions (contracts/BinaryOptionMarket.sol:314)
</code>
</pre>
#### Recommendation
While the fee calculation could be adjusted to round down to avoid reverting the transaction, a more reliable approach would be to keep the total value of accumulated fees as a state variable in the smart contract. The distribution of fees can then be calculated based on the ratio between the `poolFee` and the `creatorFee` when resolving the market.
#### Update
*Fixed in:* [c63fdcd](https://github.com/thales-markets/contracts/commit/c63fdcde53bbbed15908fde5ef9a36d19e3bde42)
The issue was addressed by accumulating and storing the fees separately from the total deposit value and calculating the fee distribution when resolving the market.
### 5.5.2 Front-runnable function (medium risk)
*[BinaryOptionMarketManager.sol#L271](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketManager.sol#L271)*
#### Description
When markets have passed their expiry date, any user could expire the markets by calling `BinaryOptionMarketManager.expireMarkets()`. Once a market has expired, any remaining or unexercised sUSD, as well as any ETH accidentally sent to the contract will be transferred to the caller.
This function had no mechanism to prevent front running. Consequently, it is expected that even unsophisticated bots monitoring the mempool would be able to detect the opportunity and front-run the transaction.
#### Recommendation
The `expireMarkets()` function should have some form of access control to prevent front-running, or the implementation should be changed to rather transfer all remaining assets to the pool.
#### Update
*Fixed in:* [a59fd2a](https://github.com/thales-markets/contracts/commit/a59fd2a64315159e83ac8d99096fc39a754a8967)
The issue was remediated by adding the `onlyOwner` modifier to the `expireMarkets()` function.
### 5.5.3 Validate market prior to external call (informational)
*[BinaryOptionMarketManager.sol#L276](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketManager.sol#L276)*
#### Description
The statement order in `BinaryOptionMarketManager.expireMarkets()` could be changed to first validate the existence of a market prior to performing an external call to a user controlled address. This would reduce the overall attack surface of the function and avoid potential future pitfalls.
#### Recommendation
The function should first attempt to remove the market from the storage array and only then call the `expire` function, as shown below:
<pre>
<code class="language-solidity" >
function expireMarkets(address[] calldata markets) external notPaused {
for (uint i = 0; i < markets.length; i++) {
address market = markets[i];
// Note that we required that the market is known, which guarantees
// its index is defined and that the list of markets is not empty.
_maturedMarkets.remove(market);
// The market itself handles decrementing the total deposits.
BinaryOptionMarket(market).expire(msg.sender);
emit MarketExpired(market);
}
}
</code>
</pre>
#### Update
*Fixed in:* [b3aab26](https://github.com/thales-markets/contracts/commit/b3aab26020f6cef57a30d79748b81cb387b6293c)
Changes were made to validate the market address before calling the external contract.
### 5.5.4 Missing function events (informational)
*[BinaryOptionMarketFactory.sol#L53](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketFactory.sol#L53), [BinaryOptionMarketFactory.sol#L57](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketFactory.sol#L57), [BinaryOptionMarketFactory.sol#L61](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketFactory.sol#L61)*
#### Description
The functions `setBinaryOptionMarketManager()`, `setBinaryOptionMarketMastercopy()` and `setBinaryOptionMastercopy()` in the `BinaryOptionMarketFactory` smart contract did not emit any events when executed. Events enhance the visibility of contract state changes; they can be used on the dApp frontend, and could also be useful for users.
#### Recommendation
Events should be added to the affected functions to emit the state changes that are made.
#### Update
*Fixed in:* [3be6698](https://github.com/thales-markets/contracts/commit/3be66982db59a0ee01cb8c74c10f16e7c9114910)
The recommended events were added to `BinaryOptionMarketFactory.sol`.
### 5.5.5 Prevent migration to self (informational)
*[BinaryOptionMarketManager.sol#L295](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketManager.sol#L295)*
#### Description
It was possible to migrate a market manager instance to itself. This did not result in unexpected behaviour, duplicate entries or overall failures, but does unnecessarily expand the attack surface of the code base.
#### Recommendation
A require statement should be added to ensure that the `receivingManager` is not the same address as the contract itself.
#### Update
*Fixed in:* [b3aab26](https://github.com/thales-markets/contracts/commit/b3aab26020f6cef57a30d79748b81cb387b6293c)
An additional `require` statement was added to `migrateMarkets()`.
### 5.5.6 Design comments (informational)
Actions to improve the functionality and readability of the codebase are outlined below.
#### Gas optimizations
* The [`BinaryOptionMarket._chooseSide()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarket.sol#L193) function is marked as internal and is only called once at [`BinaryOptionMarket.sol#L311`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarket.sol#L311). Defining a function for what could be a simple ternary statement unnecessarily increases the contract's byte code size and the overall amount of code to maintain.
* The [`BinaryOptionMarketManager.migrateMarkets()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketManager.sol#L295) function unnecessarily reiterates over the `markets` array when calling `_isKnownMarket(address(market))` and `markets.remove(address(market))`. The later statement will also revert if the array does not contain the target market. However, it is understood that the custom require message does improve usability and could be kept for this reason.
##### Update
*Fixed in:* [b3aab26](https://github.com/thales-markets/contracts/commit/b3aab26020f6cef57a30d79748b81cb387b6293c)
The team decided to not optimize `BinaryOptionMarketManager.migrateMarkets()` to maintain usability. `BinaryOptionMarket._chooseSide()` was removed and replaced with a ternary expression as recommended.
#### Dead code
The internal functions [`BinaryOptionMarket._option`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarket.sol#L204) and [`BinaryOptionMarket._subToZero`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarket.sol#L212) were unused and should be removed.
##### Update
*Fixed in:* [b3aab26](https://github.com/thales-markets/contracts/commit/b3aab26020f6cef57a30d79748b81cb387b6293c)
#### Public functions that could be external
A number of public functions could be changed to have external visbility. The arguments for public functions are copied to EVM memory, while the arguments for external functions are read from calldata. This means that external functions typically use less gas when called from another contract, and thus an external visibility should be used for any function that will only be called externally.
The following functions did were not called internally and should be changed to `external`:
* [`BinaryOption.initialize()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOption.sol#L40)
* [`BinaryOptionMarket.initialize()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarket.sol#L68)
* [`BinaryOptionMarketData.getMarketParameters()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketData.sol#L51)
* [`BinaryOptionMarketData.getMarketData()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketData.sol#L69)
* [`BinaryOptionMarketData.getAccountMarketData()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketData.sol#L84)
* [`BinaryOptionMarketFactory.setBinaryOptionMarketManager()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketFactory.sol#L53)
* [`BinaryOptionMarketFactory.setBinaryOptionMarketMastercopy()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketFactory.sol#L57)
* [`BinaryOptionMarketFactory.setBinaryOptionMastercopy()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketFactory.sol#L61)
* [`BinaryOptionMarketManager.setBinaryOptionsMarketFactory()`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketManager.sol#L93)
##### Update
*Fixed in:* [b3aab26](https://github.com/thales-markets/contracts/commit/b3aab26020f6cef57a30d79748b81cb387b6293c)
#### Use of `now` keyword
[In Solidity version 0.7.0, the `now` keyword was deprecated.](https://solidity.readthedocs.io/en/v0.7.0/070-breaking-changes.html) Developers are encouraged to use `block.timestamp` instead, to ensure forward compatibility.
The `now` keyword was used at the following locations in the code:
* [`BinaryOptionMarket.sol#L125`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarket.sol#L125)
* [`BinaryOptionMarket.sol#L129`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarket.sol#L129)
* [`BinaryOptionMarket.sol#L227`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketManager.sol#L227)
* [`BinaryOptionMarketManager.sol#L230`](https://github.com/thales-markets/contracts/blob/1eba4d10115e2fb8caa8a3f43f828c9362221b61/contracts/BinaryOptionMarketManager.sol#L230)
##### Update
*Fixed in:* [b3aab26](https://github.com/thales-markets/contracts/commit/b3aab26020f6cef57a30d79748b81cb387b6293c)
