Staked Thales Betting Proxy Smart Contract Audit

<defs>

</g>

</symbol>

</defs>

</svg>

</svg>

</div>

</svg>

<div class="frontpage-title">Staked Thales Betting Proxy Smart Contract Audit</div>

<div class="frontpage-subtitle">Thales, 11 September 2024</div>

<h1>Contents</h1>

<ul>

<li>

<a href="#introduction">Introduction</a></li>

<li>

<a href="#disclaimer">Disclaimer</a></li>

<li>

<a href="#methodology">Methodology</a></li>

<li>

<a href="#audit-findings">Audit findings</a><ul>

<li>

<a href="#io-thl-tbp-001-precision-loss-could-allow-for-draining-of-entire-reward-pool">IO-THL-TBP-001 Precision loss could allow for draining of entire reward pool</a></li>

<li>

<a href="#io-thl-tbp-002-betting-proxy-could-be-abused-to-bypass-unstake-cooldown-period">IO-THL-TBP-002 Betting proxy could be abused to bypass unstake cooldown period</a></li>

<li>

<a href="#io-thl-tbp-003-no-validation-of-collateral-type-for-live-trades">IO-THL-TBP-003 No validation of collateral type for live trades</a></li>

<li>

<a href="#io-thl-tbp-004-merge-account-does-not-check-for-active-tickets-on-source-account">IO-THL-TBP-004 Merge account does not check for active tickets on source account</a></li>

</ul>

</li>

<li>

<a href="#code-quality-improvement-suggestions">Code quality improvement suggestions</a></li>

<li>

<a href="#specification">Specification</a><ul>

<li>

<a href="#staking-thales-betting-proxy">Staking Thales Betting Proxy</a></li>

<li>

<a href="#staking-thales-changes">Staking Thales Changes</a></li>

<li>

<a href="#other-changes">Other Changes</a></li>

</ul>

</li>

<li>

<a href="#test-coverage-report">Test coverage report</a></li>

</ul>

</nav>

<h1 id="introduction">Introduction</h1>

<p>iosiro was commissioned by Thales to perform an audit of their new Staked Thales Betting Proxy smart contract and changes made to other related contracts to facilitate this new betting mechanism. The audit was conducted between the 2nd and the 5th of September 2024, using 4 audit days.</p>

<h4 id="overview">Overview</h4>

<p>The audit uncovered one high-risk issue which was subsequently remediated. This issue would have allowed malicious stakers to claim more than their share of rewards from the <code>StakingThales</code> contract. Several low-risk issues as well as code quality and design suggestions were found and are detailed in this report. A breakdown of the issues reported and their status at the conclusion of the audit is provided below:</p>

<thead>

<tr>

<th>Critical</th>

<th>Medium</th>

</tr>

</thead>

<tbody>

<tr>

</tr>

<tr>

<td>Resolved</td>

</tr>

<tr>

<td>Closed</td>

</tr>

</tbody>

</table>

<h4 id="scope">Scope</h4>

<p>The assessment focused on source file listed below, with all other files considered out of scope. Any out-of-scope code interacting with the assessed code was presumed to operate correctly without introducing functional or security vulnerabilities.</p>

<ul>

<li><strong>Project name:</strong> thales-markets/contracts and thales-markets/contracts-v2</li>

<li><strong>Initial audit commit:</strong> <a href="https://github.com/thales-markets/contracts-v2/tree/012072f78e4fdb57db78591036582729e41cb660">012072f</a> and <a href="https://github.com/thales-markets/contracts/tree/20c51000f5003d1edfec565d4151030a9219b308">20c5100</a></li>

<li><strong>Final review commit:</strong> <a href="https://github.com/thales-markets/contracts-v2/tree/b0219e33240ed65cab9d1a03a9b42c9e061586d8">b0219e3</a> and <a href="https://github.com/thales-markets/contracts/tree/82c756309278eda3197fe75dd012cb077e6e95a2">82c7563</a></li>

<li><strong>Files:</strong> <code>contracts/contracts/EscrowAndStaking/StakingThales.sol</code>, <code>contracts-v2/contracts/core/AMM/SportsAMMV2.sol</code>, <code>contracts-v2/contracts/core/StakingThalesBetting/StakingThalesBettingProxy.sol</code>, <code>contracts-v2/contracts/core/LiveTrading/LiveTradingProcessor.sol</code></li>

</ul>

<p>A specification is available in the <a href="#specification">Specification section</a> of this report.</p>

<h1 id="disclaimer">Disclaimer</h1>

<p>This report aims to provide an overview of the assessed smart contracts' risk exposure and a guide to improving their security posture by addressing identified issues. The audit, limited to specific source code at the time of review, sought to:</p>

<ul>

<li>Identify potential security flaws.</li>

<li>Verify that the smart contracts' functionality aligns with their documentation.</li>

</ul>

<p>Off-chain components, such as backend web application code, keeper functionality, and deployment scripts were explicitly not in-scope of this audit.</p>

<p>Given the unregulated nature and ease of cryptocurrency transfers, operations involving these assets face a high risk from cyber attacks. Maintaining the highest security level is crucial, necessitating a proactive and adaptive approach that accounts for the experimental and rapidly evolving nature of blockchain technology. To encourage secure code development, developers should:</p>

<ul>

<li>Integrate security throughout the development lifecycle.</li>

<li>Employ defensive programming to mitigate the risks posed by unexpected events.</li>

<li>Adhere to current best practices wherever possible.</li>

</ul>

<h1 id="methodology">Methodology</h1>

<p>The audit was conducted using the techniques described below.</p>

<dl>

<dt>Code review</dt>

<dd>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.</dd>

<dt>Dynamic analysis</dt>

<dd>The contracts were compiled, deployed, and tested in a test environment, both manually and through the test suite provided. Dynamic analysis was used to identify additional edge cases, confirm that the code was functional, and to validate the reported issues.</dd>

<dt>Automated analysis</dt>

<dd>Automated tooling was used to detect the presence of various types of security vulnerabilities. Static analysis results were reviewed manually and any false positives were removed. Any true positive results are included in this report.</dd>

</dl>

<h1 id="audit-findings">Audit findings</h1>

<p>The table below provides an overview of the audit's findings. Detailed write-ups are provided below.</p>

<thead>

<tr>

<th>Issue</th>

<th>Status</th>

</tr>

</thead>

<tbody>

<tr>

<td>Precision loss could allow for draining of entire reward pool</td>

<td class="status-resolved">Resolved</td>

</tr>

<tr>

<td>Betting proxy could be abused to bypass unstake cooldown period</td>

<td class="status-closed">Closed</td>

</tr>

<tr>

<td>No validation of collateral type for live trades</td>

<td class="status-resolved">Resolved</td>

</tr>

<tr>

<td>Merge account does not check for active tickets on source account</td>

<td class="status-closed">Closed</td>

</tr>

</tbody>

</table>

<p>Each issue identified during the audit has been assigned a risk rating. The rating is determined based on the criteria outlined below.</p>

<dl>

<dt>Critical risk</dt>

<dd>The issue could result in the theft of funds from the contract or its users.</dd>

<dd>The issue could result in the loss of funds for the contract owner or its users.</dd>

<dt>Medium risk</dt>

<dd>The issue resulted in the code being dysfunctional or the specification being implemented incorrectly.</dd>

<dd>A design or best practice issue that could affect the ordinary functioning of the contract.</dd>

<dt>Informational</dt>

<dd>An improvement related to best practice or a suboptimal design pattern.</dd>

</dl>

<p>In addition to a risk rating, each issue is assigned a status:</p>

<dl>

<dd>The issue remained present in the code as of the final commit reviewed and may still pose a risk.</dd>

<dt>Resolved</dt>

<dd>The issue was identified during the audit and has since been satisfactorily addressed, removing the risk it posed.</dd>

<dt>Closed</dt>

<dd>The issue was identified during the audit and acknowledged by the developers as an acceptable risk without actioning any change.</dd>

</dl>

<a name="IO-THL-TBP-001"></a><h2 id="io-thl-tbp-001-precision-loss-could-allow-for-draining-of-entire-reward-pool" class="break-before"><strong>IO-THL-TBP-001</strong> Precision loss could allow for draining of entire reward pool</h2>

<tbody>

<tr>

<td class="status-resolved">Resolved</td>

<td><a href="https://github.com/thales-markets/contracts/blob/20c51000f5003d1edfec565d4151030a9219b308/contracts/EscrowAndStaking/StakingThales.sol#L652-L653">StakingThales.sol#L652-L653</a></td>

</tr>

</tbody>

</table>

<p>The <code>StakingThales</code> contract enforces the claiming of rewards before any modifications to an account’s staking balance. This is done by calling <code>StakingThales::_calculateAvailableRewardsToClaim()</code> and, if the amount of rewards available to claim is non-zero, call <code>StakingThales::_claimReward()</code>. Once the rewards are claimed, the <code>_lastRewardsClaimedPeriod</code> value for the account is updated to the current rewards period. This prevents the account from claiming more than once per period. This control is also intended to ensure that an account cannot increase its staked balance and immediately claim rewards in the current period, as rewards due are calculated as a proportion of the total amount staked and escrowed in the previous period.</p>

<p>The snippet below from <code>StakingThales::getBaseReward()</code> shows how the base reward value, that forms the basis of available rewards, is calculated for an account:</p>

<pre tabindex="0" class="chroma"><code><span class="line"><span class="cl"><span class="n">_baseRewards</span> <span class="o">=</span> <span class="n">_stakedBalances</span><span class="p">[</span><span class="n">account</span><span class="p">]</span>

</span></span><span class="line"><span class="cl"><span class="p">.</span><span class="n">add</span><span class="p">(</span><span class="n">iEscrowThales</span><span class="p">.</span><span class="n">getStakedEscrowedBalanceForRewards</span><span class="p">(</span><span class="n">account</span><span class="p">))</span>

</span></span><span class="line"><span class="cl"><span class="p">.</span><span class="n">mul</span><span class="p">(</span><span class="n">currentPeriodRewards</span><span class="p">)</span>

</span></span><span class="line"><span class="cl"><span class="p">.</span><span class="n">div</span><span class="p">(</span><span class="n">totalStakedLastPeriodEnd</span><span class="p">.</span><span class="n">add</span><span class="p">(</span><span class="n">totalEscrowedLastPeriodEnd</span><span class="p">));</span>

</span></span></code></pre><p>Should the current staked balance be a small value, such as 1 wei, then the base reward value would be zero in most cases where <code>currentPeriodRewards</code>, <code>totalStakedLastPeriodEnd</code>, and <code>totalEscrowedLastPeriodEnd</code> are 18 decimal values. As a result <code>_calculateAvailableRewardsToClaim()</code> will also return zero and the logic within the following if block will not execute:</p>

<pre tabindex="0" class="chroma"><code><span class="line"><span class="cl"><span class="k">if</span> <span class="p">(</span><span class="n">_calculateAvailableRewardsToClaim</span><span class="p">(</span><span class="n">_account</span><span class="p">)</span> <span class="o">></span> <span class="mi">0</span><span class="p">)</span> <span class="p">{</span>

</span></span><span class="line"><span class="cl"> <span class="n">_claimReward</span><span class="p">(</span><span class="n">_account</span><span class="p">);</span>

</span></span><span class="line"><span class="cl"><span class="p">}</span>

</span></span></code></pre><p>Consequently, <code>_lastRewardsClaimedPeriod</code> will not be updated and it would be possible to increase an account’s staked balance from a small initial value and then immediately claim rewards against the new larger balance. As these rewards are proportional to the current staked amount and the amount escrowed for the last reward period, users could receive more rewards than they should.</p>

<p>A malicious user could abuse this to claim the entire <code>feeToken</code> balance of the <code>StakingThales</code> contract, preventing other users from claiming rewards. It may also be possible to do the same with the THALES token balance of the <code>ThalesStakingRewardsPool</code>.</p>

<h3 id="recommendation">Recommendation</h3>

<p>The if statement in <code>StakingThales::_modifyStakingBalance()</code> that calculates available rewards should instead check whether <code>_lastRewardsClaimedPeriod[account] != periodsOfStaking</code> before calling <code>_claimRewards()</code> . This will ensure that <code>_lastRewardsClaimedPeriod</code> for an account is always updated before sensitive actions, such as staking and unstaking, while also distributing any applicable rewards. To improve readability, this if block could be put in a modifier that can then be applied across all functions that modify an account’s balance.</p>

<p>Alternatively, <code>_lastRewardsClaimedPeriod</code> for an account should always be set to the <code>periodsOfStaking</code> before any modifications to staking balances are made.</p>

<h3 id="client-response">Client response</h3>

<p>Fixed in <a href="https://github.com/thales-markets/contracts/pull/475/commits/914415cc0c8002c68f3cb9dcfb736e7beee5a548">914415c</a>.</p>

<a name="IO-THL-TBP-002"></a><h2 id="io-thl-tbp-002-betting-proxy-could-be-abused-to-bypass-unstake-cooldown-period" class="break-before"><strong>IO-THL-TBP-002</strong> Betting proxy could be abused to bypass unstake cooldown period</h2>

<tbody>

<tr>

<td class="status-closed">Closed</td>

<td><a href="https://github.com/thales-markets/contracts-v2/blob/012072f78e4fdb57db78591036582729e41cb660/contracts/core/StakingThalesBetting/StakingThalesBettingProxy.sol">StakingThalesBettingProxy.sol</a></td>

</tr>

</tbody>

</table>

<p>The new staked THALES betting mechanism could provide a way for malicious users to bypass the unstake cooldown period by making a delta neutral bet that will most likely result in a loss for the staked account. By placing a bet on the opposite side of the market to their staked account, the user stands to win back liquid THALES.</p>

<p>This issue is inherent in the design of this new mechanism. Based on the limited review of <code>StakingThales</code> and related contracts such as <code>EscrowThales</code>, its impact at the time of this audit is thought to be limited, as users are exposed to the risk of losing the position and shifting liquid tokens into staked tokens. However, the issue has been raised to ensure it is known to the team and taken to account during future development of <code>StakingThales</code>.</p>

<h3 id="recommendation-1">Recommendation</h3>

<p>The potential for the unstake cooldown period to be bypassed should be considered in any future changes to <code>StakingThales</code> and related contracts that rely on the unstake cooldown period as a security control.</p>

<h3 id="client-response-1">Client response</h3>

<p>Acknowledged by Thales as per design. Furthermore, the attacker would likely lose some amount to margin if they choose this route to fast-track unstaking.</p>

<a name="IO-THL-TBP-003"></a><h2 id="io-thl-tbp-003-no-validation-of-collateral-type-for-live-trades" class="break-before"><strong>IO-THL-TBP-003</strong> No validation of collateral type for live trades</h2>

<tbody>

<tr>

<td class="status-resolved">Resolved</td>

</tr>

</tbody>

</table>

<p>There is no check in <code>StakingThalesBettingProxy::tradeLive</code> that the <code>_collateral</code> in <code>_liveTradeData</code> is the <code>stakingCollateral</code>, before the trade is submitted. This issue is currently not exploitable, as it would only result in a revert when <code>SportsAMMV2</code> attempts a transfer of the specified collateral from the betting proxy. However, should new features be added to the <code>StakingThalesBettingProxy</code> contract that results in it approving <code>SportsAMMV2</code> for other collateral, it would be possible to create bets using that collateral.</p>

<h3 id="recommendation-2">Recommendation</h3>

<p>Validate that the <code>_collateral</code> in <code>_liveTradeData</code> is the <code>stakingCollateral</code>, and revert if this is not the case.</p>

<h3 id="client-response-2">Client response</h3>

<p>Fixed in <a href="https://github.com/thales-markets/contracts-v2/pull/75/commits/78d5a6b47213c4f272bb17e9da989aae6385b5bb">78d5a6b</a>.</p>

<a name="IO-THL-TBP-004"></a><h2 id="io-thl-tbp-004-merge-account-does-not-check-for-active-tickets-on-source-account" class="break-before"><strong>IO-THL-TBP-004</strong> Merge account does not check for active tickets on source account</h2>

<tbody>

<tr>

<td class="status-closed">Closed</td>

<td><a href="https://github.com/thales-markets/contracts/blob/20c51000f5003d1edfec565d4151030a9219b308/contracts/EscrowAndStaking/StakingThales.sol#L680">StakingThales.sol#L680</a></td>

</tr>

</tbody>

</table>

<p><code>StakingThales::mergeAccount()</code> does not check whether the source account has any existing active tickets in <code>StakingThalesBettingProxy</code> before it allows merging of that account with the specified <code>destAccount</code>. This could result in unintentional loss of funds should those tickets be resolved and winnings distributed to an account that has since been merged with another account.</p>

<h3 id="recommendation-3">Recommendation</h3>

<p><code>StakingThales::mergeAccount()</code> should check whether the source account has any existing active tickets in <code>StakingThalesBettingProxy</code> and revert if this is the case.</p>

<h3 id="client-response-3">Client response</h3>

<p>Won't fix. The current implementation is intentional as <code>StakingThales::mergeAccount()</code> is usually called in high-urgency situations e.g. when a user's private key has been stolen. Thales does not want to block this action while waiting for pending tickets to be resolved.</p>

<h1 id="code-quality-improvement-suggestions">Code quality improvement suggestions</h1>

<p>Code improvement suggestions without security implications are listed below.</p>

<thead>

<tr>

<th>Location</th>

<th>Details</th>

</tr>

</thead>

<tbody>

<tr>

<td><a href="https://github.com/thales-markets/contracts/blob/20c51000f5003d1edfec565d4151030a9219b308/contracts/EscrowAndStaking/StakingThales.sol#L319">StakingThales.sol#L319</a></td>

<td>The allowance of THALES set in the <code>StakingThales</code> contract when setting the <code>EscrowThales</code> contract is no longer necessary as the rewards are deposited and pulled from the <code>ThalesStakingRewardsPool</code> contract and not the <code>StakingThales</code> contract. Removing the allowance would mitigate the impact of any potential issues in the <code>EscrowThales</code> that could transfer THALES token from <code>StakingThales</code>.</td>

</tr>

<tr>

<td><a href="https://github.com/thales-markets/contracts/blob/20c51000f5003d1edfec565d4151030a9219b308/contracts/EscrowAndStaking/StakingThales.sol">StakingThales.sol</a></td>

<td>To prevent the accumulation of unused storage variables as is currently the case in <code>StakingThales.sol</code>, namespaced storage libraries should be considered for this and other contracts in future development.</td>

</tr>

<tr>

<td>General</td>

<td>Defining a common interface for betting proxies that encompasses the common functions in the <code>stakingThalesBettingProxy</code> and <code>FreeBetsHolder</code> would simplify the code in <code>SportsAMMV2</code> and <code>LiveTradingProcessor</code> as it would no longer be necessary to have if/else branches for calling functions such as <code>confirmTicketResolved()</code>. Additionally, this will reduce the number of changes needed for any future betting proxies.</td>

</tr>

<tr>

<td><code>liveRequestsPerUser</code> should be renamed to <code>liveRequestsToUser</code> to be more consistent with the naming scheme of the similar <code>ticketToUser</code> mapping in the contract</td>

</tr>

</tbody>

</table>

<p>These suggestions were communicated to the team resulting in the following:</p>

<ol>

<li>Fixed in <a href="https://github.com/thales-markets/contracts/commit/0f7727dfcac91568b2fc737837382651267f130c">0f7727d</a></li>

<li>Acknowledged and will be taken into account for future development</li>

<li>Fixed in <a href="https://github.com/thales-markets/contracts-v2/commit/027b01741e03441e614365ccfe2a9681366a9a1d">027b017</a></li>

<li>Won't fix to maintain consistency in naming convention with the same variable in the <code>FreeBetsHolder</code> contract.</li>

</ol>

<h1 id="specification">Specification</h1>

<p>The following section outlines the functional changes to the system at a high level as implemented in the codebase. Any perceived points of conflict should be highlighted with the auditing team to determine the source of the discrepancy.</p>

<h2 id="staking-thales-betting-proxy">Staking Thales Betting Proxy</h2>

<p>Thales introduced a new mechanism through which accounts who had staked THALES in the <code>StakingThales</code> contract could use their staked balance to make trades via the <code>SportsAMMV2</code> contract. This was done through the introduction of a new <code>StakingThalesBettingProxy</code> contract which exposed proxy functions to make normal and live trades. This contract bundles the logic of withdrawing the THALES from the staker's balance in <code>StakingThales</code>, creating a trade in <code>SportsAMMV2</code>, keeping records of active and resolved tickets, and depositing any winnings into the staker's balance in <code>StakingThales</code>.</p>

<h2 id="staking-thales-changes">Staking Thales Changes</h2>

<p>Changes were made to the existing <code>StakingThales</code> contract to facilitate the new betting proxy. Primarily, a new <code>_modifyStakingBalance()</code> internal function was introduced to ensure the same accounting logic is applied when staking, unstaking, or buying or redeeming tickets via the <code>StakingThalesBettingProxy</code>.</p>

<h2 id="other-changes">Other Changes</h2>

<p>Several small changes were made to the <code>SportsAMMV2</code> and <code>liveTradingProcessor</code> contracts. These changes introduced new hooks to allow the <code>StakingThalesBettingProxy</code> to make normal and live trades, withdraw the required THALES from <code>StakingThales</code> at the appropriate time, and perform post-ticket resolution housekeeping e.g. deposit any winnings back to <code>StakingThales</code>.</p>

<h1 id="test-coverage-report">Test coverage report</h1>

<p>The coverage report of the provided tests as on the final day of the audit is given below.</p>

<table>

<thead>

<tr>

<th>% Stmts</th>

<th>% Branch</th>

<th>% Funcs</th>

<th>% Lines</th>

<th>Uncovered Lines</th>

</tr>

</thead>

<tbody>

<tr>

</tr>

<tr>

<td>SportsAMMV2.sol</td>

</tr>

<tr>

<td>SportsAMMV2Manager.sol</td>

</tr>

<tr>

<td>SportsAMMV2RiskManager.sol</td>

</tr>

<tr>

<td>Ticket.sol</td>

</tr>

<tr>

<td>TicketMastercopy.sol</td>

</tr>

<tr>

</tr>

<tr>

<td>SportsAMMV2Data.sol</td>

</tr>

<tr>

<td>SportsAMMV2LiquidityPoolData.sol</td>

</tr>

<tr>

<td>core/FreeBets/</td>

</tr>

<tr>

<td>FreeBetsHolder.sol</td>

</tr>

<tr>

<td>core/LiquidityPool/</td>

</tr>

<tr>

<td>DefaultLiquidityProvider.sol</td>

</tr>

<tr>

<td>SportsAMMV2LiquidityPool.sol</td>

</tr>

<tr>

<td>SportsAMMV2LiquidityPoolRound.sol</td>

</tr>

<tr>

<td>SportsAMMV2LiquidityPoolRoundMastercopy.sol</td>

</tr>

<tr>

<td>core/LiveTrading/</td>

</tr>

<tr>

<td>LiveTradingProcessor.sol</td>

</tr>

<tr>

<td>core/Resolving/</td>

</tr>

<tr>

<td>ChainlinkResolver.sol</td>

</tr>

<tr>

<td>SportsAMMV2ResultManager.sol</td>

</tr>

<tr>

<td>core/StakingThalesBetting/</td>

</tr>

<tr>

<td>StakingThalesBettingProxy.sol</td>

</tr>

<tr>

<td>interfaces/</td>

</tr>

<tr>

<td>IChainlinkResolver.sol</td>

</tr>

<tr>

<td>IFreeBetsHolder.sol</td>

</tr>

<tr>

<td>ILiveTradingProcessor.sol</td>

</tr>

<tr>

<td>IProxyBetting.sol</td>

</tr>

<tr>

<td>ISportsAMMV2.sol</td>

</tr>

<tr>

<td>ISportsAMMV2LiquidityPool.sol</td>

</tr>

<tr>

<td>ISportsAMMV2Manager.sol</td>

</tr>

<tr>

<td>ISportsAMMV2ResultManager.sol</td>

</tr>

<tr>

<td>ISportsAMMV2RiskManager.sol</td>

</tr>

<tr>

<td>IStakingThalesBettingProxy.sol</td>

</tr>

<tr>

<td>ITicket.sol</td>

</tr>

<tr>

<td>IWeth.sol</td>

</tr>

<tr>

<td>All files</td>

</tr>

</tbody>

</table>

<p>Overall, the test quality of the smart contracts in scope was comprehensive.</p>

</article>

</main>

</div>