The current implementation allows users to make withdraw requests for future epochs without properly accounting for pending withdrawals in the current epoch. This can result in a situation where the total withdraw requests exceed the user's actual balance.

The following snippet shows that the totalSharesBeingWithdrawn function only checks next epoch's withdraw requests:

function requestWithdraw(uint256 shares, address owner) public {
    if (msg.sender != owner && shares > allowance[owner][msg.sender]) {
        revert IErrors.NotAllowed();
    }
    if (totalSharesBeingWithdrawn(owner) + shares > balanceOf[owner]) {
        revert IErrors.BalanceExceeded();
    }

    uint256 unlockEpoch = currentEpoch + 1;
    withdrawRequests[owner][unlockEpoch] += shares;
    totalWithdrawRequests[unlockEpoch] += shares;

    emit WithdrawRequested(msg.sender, owner, shares, currentEpoch, unlockEpoch);
}

function totalSharesBeingWithdrawn(address owner) public view returns (uint256 shares) {
    return withdrawRequests[owner][currentEpoch + 1];
}

Example scenario:

• At epoch 0, a user with 1000 shares requests to withdraw 1000 shares for the next epoch (epoch 1).

• In epoch 1, without withdrawing, the user requests another 1000 shares withdrawal for the following epoch (epoch 2).

• The contract allows this second request because it only checks the sum of current requests (0 for epoch 2) and the new request (1000) against the user's balance (1000).

• Now the contract shows withdraw requests of 1000 shares for both epoch 1 and epoch 2, despite the user only having 1000 shares total.

This leads to state discrepancies, wrong availableBalance (the function does not account for funds that can be withdrawn in the current epoch, leading to inaccurate reporting), and potential confusions misleading administrators about the actual funds needed for withdrawals.

While this discrepancy exists, the contract does prevent users from withdrawing more than they own. Any attempt to withdraw beyond the user's actual balance will fail.

The following PoC shows that Alice can request withdraws in two epochs, although she cannot withdraw more than entitled to, this can give a wrong signal to the administrator that will want to make sure that enough funds are necessary to withdraw. Additionally, it also shows that the availableAssets() function does not take into account the funds that can be withdrawn in the current epoch.

function test_halborn_commonPool_withdraw() public {
    uint256 deposit_amount = 100e6;
    uint256 expected_shares_amount_after_deposit = deposit_amount;
    uint256 withdraw_amount = expected_shares_amount_after_deposit;
    address alice = vm.addr(0xA11CE);
    address bob = vm.addr(0xB0B);
    uint256 lastEpoch = 0; // will update later
    uint256 currentEpoch = commonPool.currentEpoch();
    uint256 nextEpoch = currentEpoch + 1;
    uint256 totalPoolAssetsAfterDeposit = 0;
    uint256 availableAssetsAfterWithdrawRequest = 0;
    uint256 balanceOfCommonPool = 0;

    // ------------------ First Epoch ------------------
    // Alice and Bob deposit 100e6 USDC each into the pool
    // -------------------------------------------------

    // This deals USDC tokens, approve and deposit into the CommonPool
    vm.startPrank(alice);
    _depositIntoPool(commonPool, USDC_TOKEN_ARB, deposit_amount, alice, alice);
    assertEq(commonPool.balanceOf(alice), expected_shares_amount_after_deposit);
    vm.startPrank(bob);
    _depositIntoPool(commonPool, USDC_TOKEN_ARB, deposit_amount, bob, bob);
    console2.log(""[+] Alice and Bob deposited 100e6 USDC each into the pool"");

    balanceOfCommonPool = IERC20(USDC_TOKEN_ARB).balanceOf(address(commonPool));
    console2.log(""[+] CommonPool has "", balanceOfCommonPool, "" USDC"");
    assertEq(balanceOfCommonPool, deposit_amount * 2);

    // Warp to next epoch so that nav is updated
    assertEq(commonPool.totalAssets(), 0); // Assets are 0 even though we deposited because we need to wait for the NAV update through swapExcessAndRecalculate
    vm.warp(block.timestamp + commonPool.EPOCH_DURATION() + 1);
    commonPool.endCurrentEpoch();
    lastEpoch = currentEpoch;
    currentEpoch = commonPool.currentEpoch();
    nextEpoch = currentEpoch + 1;
    console2.log(""[+] Epoch ended after the EPOCH_DURATION"");
    // No need to send more USDC since we deposited 200 already, and we dont change epoch since we did that already
    vm.startPrank(address(this));
    _setPoolValue(commonPool, deposit_amount * 2, false); // We still specify 2*deposit because there is a vm.deal that sets the amount as the balance
    console2.log(commonPool.totalAssets());
    vm.startPrank(alice);

    // ------------------ Second Epoch ------------------
    // Alice requests a withdraw of 100e6 USDC
    // -------------------------------------------------

    // Let's see the available assets in the pool
    totalPoolAssetsAfterDeposit = commonPool.totalAssets();
    console2.log(""[+] CommonPool has "", totalPoolAssetsAfterDeposit, "" total assets (USDC)"");
    assertEq(commonPool.availableAssets(), totalPoolAssetsAfterDeposit);
    console2.log(""[+] CommonPool has "", totalPoolAssetsAfterDeposit, "" available assets (USDC)"");

    // Alice tries to withdraw and that fails because she didn't request a withdraw
    vm.startPrank(alice);
    vm.expectRevert();
    commonPool.withdraw(withdraw_amount, alice, alice);
    console2.log(""[+] Alice cannot withdraw without requesting a withdraw"");

    // Alice cannot withdraw more shares than she has
    vm.expectRevert();
    commonPool.requestWithdraw(expected_shares_amount_after_deposit + 1, alice);
    console2.log(""[+] Alice cannot withdraw more shares than she has"");

    // Alice requests a withdraw
    commonPool.requestWithdraw(withdraw_amount, alice);
    console2.log(""[+] Alice requested a withdraw"");

    // Make sure commonPool.withdrawRequests[owner][nextEpoch] is withdraw_amount
    assertEq(commonPool.withdrawRequests(alice, nextEpoch), withdraw_amount);
    assertEq(commonPool.totalWithdrawRequests(nextEpoch), withdraw_amount);
    console2.log(""[+] Alice's withdraw request is in the next epoch"");
    availableAssetsAfterWithdrawRequest = commonPool.availableAssets();
    console2.log(""[+] CommonPool has "", availableAssetsAfterWithdrawRequest, "" available assets (USDC)"");

    // Alice cannot end epoch just now (admin can)
    vm.expectRevert();
    commonPool.endCurrentEpoch();
    console2.log(""[+] Alice cannot end epoch just now"");

    // Warp to next epoch
    vm.warp(block.timestamp + commonPool.EPOCH_DURATION() + 1);
    commonPool.endCurrentEpoch();
    lastEpoch = currentEpoch;
    currentEpoch = commonPool.currentEpoch();
    nextEpoch = currentEpoch + 1;
    console2.log(""[+] Epoch ended after the EPOCH_DURATION"");

    // ------------------ Third Epoch ------------------
    // Alice withdraws 100e6 USDC
    // Bob requests a withdraw of 100e6 USDC
    // -------------------------------------------------

    // This should be the same as the previous epoch, but it is not because the current withdraws are not taken into account
    assertNotEq(commonPool.availableAssets(), availableAssetsAfterWithdrawRequest); // <------------------------THIS SHOULD BE FAILING

    // Let's see the available assets in the pool
    console2.log(""[+] CommonPool has "", commonPool.totalAssets(), "" total assets (USDC)"");
    console2.log(""[+] CommonPool has "", commonPool.availableAssets(), "" available assets (USDC)"");

    // No need to send more USDC since we deposited 200 already, and we dont change epoch since we did that already
    vm.startPrank(address(this));
    _setPoolValue(commonPool, deposit_amount * 2, false); // We still specify 2*deposit because there is a vm.deal that sets the amount as the balance
    console2.log(commonPool.totalAssets());
    vm.startPrank(alice);

    // Check that the withdraw request is still there
    assertEq(commonPool.withdrawRequests(alice, currentEpoch), withdraw_amount);
    assertEq(commonPool.totalWithdrawRequests(currentEpoch), withdraw_amount);
    console2.log(""[+] Alice's withdraw request is now in the current epoch"");

    // Alice cannot withdraw more shares than she has
    vm.expectRevert();
    commonPool.withdraw(expected_shares_amount_after_deposit + 1, alice, alice);
    console2.log(""[+] Alice cannot withdraw more shares than she has"");

    // Now we show that alice can still ask another withdraw even though she has one pending
    commonPool.requestWithdraw(withdraw_amount, alice);
    console2.log(""[+] Alice requested another withdraw (but she already has one pending)"");

    // Bob can also ask for a withdraw
    vm.startPrank(bob);
    commonPool.requestWithdraw(withdraw_amount, bob);
    vm.startPrank(alice);
    console2.log(""[+] Bob also requested a withdraw"");

    // Show the new withdraw request is in the next epoch
    assertEq(commonPool.withdrawRequests(alice, currentEpoch), withdraw_amount);
    assertEq(commonPool.withdrawRequests(bob, currentEpoch), 0);
    assertEq(commonPool.withdrawRequests(alice, nextEpoch), withdraw_amount);
    assertEq(commonPool.withdrawRequests(bob, nextEpoch), withdraw_amount);
    assertEq(commonPool.totalWithdrawRequests(currentEpoch), withdraw_amount);
    assertEq(commonPool.totalWithdrawRequests(nextEpoch), withdraw_amount * 2);
    console2.log(""[+] Alice's has now a withdraw request in the current and next epoch, Bob only in the next epoch"");

    // She now withdraws
    commonPool.withdraw(withdraw_amount, alice, alice);
    console2.log(""[+] Alice withdrew"");

    // Check that she has no more shares and that she has USDC
    assertEq(commonPool.balanceOf(alice), 0);
    assertEq(IERC20(USDC_TOKEN_ARB).balanceOf(alice), deposit_amount);

    // Spend another epoch
    vm.warp(block.timestamp + commonPool.EPOCH_DURATION() + 1);
    commonPool.endCurrentEpoch();
    lastEpoch = currentEpoch;
    currentEpoch = commonPool.currentEpoch();
    nextEpoch = currentEpoch + 1;
    console2.log(""[+] Epoch ended after the EPOCH_DURATION"");

    // Alice tries to withdraw but she cannot
    vm.expectRevert();
    commonPool.withdraw(withdraw_amount, alice, alice);
    console2.log(""[+] Alice cannot withdraw because she already withdrew and has no more shares"");
}

The following figure shows that it was possible to request multiple withdraws:

It is recommended to take the current epoch withdraw requests into account when deciding whether the user can add another withdraw request, and when wanting to transfer.

SOLVED: the RFX Exchange team solved this issue by getting rid of the epoch system and using ERC7540 standard for async redeem/withdraw requests.

The current implementation of the CommonPool contract allows approved spenders to request withdrawals on behalf of the owner. However, the contract only checks if the requested amount is lower than the allowance for each individual request, without keeping track of the cumulative amount requested. This oversight enables spenders to call the withdraw request function multiple times, potentially exceeding their initial allowance.

The following snippet shows that the allowance variable is checked without being updated:

function requestWithdraw(uint256 shares, address owner) public {
    if (msg.sender != owner && shares > allowance[owner][msg.sender]) {
        revert IErrors.NotAllowed();
    }
    if (totalSharesBeingWithdrawn(owner) + shares > balanceOf[owner]) {
        revert IErrors.BalanceExceeded();
    }

    uint256 unlockEpoch = currentEpoch + 1;
    withdrawRequests[owner][unlockEpoch] += shares;
    totalWithdrawRequests[unlockEpoch] += shares;

    emit WithdrawRequested(msg.sender, owner, shares, currentEpoch, unlockEpoch);
}

As a result, total withdraw requests for a user can be artificially inflated, and it could lead to confusion about the actual intended withdrawals and complicate the withdrawal process

Scenario:

• An owner (e.g., Alice) approves a spender (e.g., Bob) to request a withdrawal of a specific amount (e.g., 10e6 USDC).

• The spender (Bob) can then call the requestWithdraw function multiple times, each time requesting up to the allowed amount.

• The contract allows these multiple requests without decrementing the allowance or tracking the total requested amount.

While this vulnerability allows for inflated withdraw requests, the contract still prevents actual withdrawals beyond the user's balance because the shares burn attempt will revert if the caller does not have enough shares.

The following PoC shows that bob could request 20e6 withdraw requests even though his allowance was only 10e6:

function test_halborn_commonPool_requestWithdraw() public {
    uint256 deposit_amount = 100e6;
    uint256 expected_shares_amount_after_deposit = deposit_amount;
    uint256 withdraw_amount = expected_shares_amount_after_deposit;
    address alice = vm.addr(0xA11CE);
    address bob = vm.addr(0xB0B);
    uint256 lastEpoch = 0; // will update later
    uint256 currentEpoch = commonPool.currentEpoch();
    uint256 nextEpoch = currentEpoch + 1;
    uint256 totalPoolAssetsAfterDeposit = 0;
    uint256 availableAssetsAfterWithdrawRequest = 0;
    uint256 balanceOfCommonPool = 0;
    uint256 allowanceAmount = 10e6;

    // ------------------ First Epoch ------------------
    // Alice and Bob deposit 100e6 USDC each into the pool
    // -------------------------------------------------

    // This deals USDC tokens, approve and deposit into the CommonPool
    vm.startPrank(alice);
    _depositIntoPool(commonPool, USDC_TOKEN_ARB, deposit_amount, alice, alice);
    assertEq(commonPool.balanceOf(alice), expected_shares_amount_after_deposit);
    vm.startPrank(bob);
    _depositIntoPool(commonPool, USDC_TOKEN_ARB, deposit_amount, bob, bob);
    console2.log(""[+] Alice and Bob deposited 100e6 USDC each into the pool"");

    balanceOfCommonPool = IERC20(USDC_TOKEN_ARB).balanceOf(address(commonPool));
    console2.log(""[+] CommonPool has "", balanceOfCommonPool, "" USDC"");
    assertEq(balanceOfCommonPool, deposit_amount * 2);

    // Warp to next epoch so that nav is updated
    assertEq(commonPool.totalAssets(), 0); // Assets are 0 even though we deposited because we need to wait for the NAV update through swapExcessAndRecalculate
    vm.warp(block.timestamp + commonPool.EPOCH_DURATION() + 1);
    commonPool.endCurrentEpoch();
    lastEpoch = currentEpoch;
    currentEpoch = commonPool.currentEpoch();
    nextEpoch = currentEpoch + 1;
    console2.log(""[+] Epoch ended after the EPOCH_DURATION"");
    // No need to send more USDC since we deposited 200 already, and we dont change epoch since we did that already
    vm.startPrank(address(this));
    _setPoolValue(commonPool, deposit_amount * 2, false); // We still specify 2*deposit because there is a vm.deal that sets the amount as the balance
    console2.log(commonPool.totalAssets());
    vm.startPrank(alice);

    // ------------------ Second Epoch ------------------
    // Alice approves bob to request 10e6 USDC for her
    // -------------------------------------------------

    commonPool.approve(bob, allowanceAmount);
    console2.log(""[+] Alice approved Bob to request 10e6 USDC for her"");

    // Bob requests 10e6 USDC for Alice
    vm.startPrank(bob);
    commonPool.requestWithdraw(allowanceAmount, alice);
    console2.log(""[+] Bob requested 10e6 USDC for Alice"");

    // Alice approves the request
    assertEq(commonPool.withdrawRequests(alice, nextEpoch), allowanceAmount);
    console2.log(""[+] There are 10e6 USDC requested for Alice in the next epoch"");

    // But bob can still request again
    commonPool.requestWithdraw(allowanceAmount, alice);
    assertEq(commonPool.withdrawRequests(alice, nextEpoch), allowanceAmount * 2);
    console2.log(""[+] Bob requested another 10e6 USDC for Alice, the total request is now 20e6 USDC"");

}

It is recommended to keep track of the allowances used for the withdraw requests.

SOLVED: the RFX Exchange team solved this issue by implementing the ERC7540 standard and decreasing the allowance immediately in the requestRedeem function.

The last withdrawal of shares will trigger a division by 0 in _scaleVariables, and revert the transaction, meaning that it will fail to retrieve the wanted amount. It is however possible to leave 1 share into the contract and withdraw the rest.

In the following snippet, we can see that supply - shares would be 0 when trying to withdraw the last shares.

function _scaleVariables(uint256 shares, bool isDeposit) internal {
    uint256 supply = totalSupply;
    shareToAssetPrice = (shareToAssetPrice * supply) / (isDeposit ? supply + shares : supply - shares);
}

The following PoC shows that Alice cannot withdraw her whole shares from the contract.

function test_halborn_withdrawAllAssets() public {
    uint256 arbitrary_amount = 1000e6;
    uint256 alice_amount = 1000e6;
    uint256 alice_expected_shares = alice_amount;
    address alice = vm.addr(0xA11CE);
    address bob = vm.addr(0xB0B);
    address from = address(this);

    // Someone sends 1000e6 USDC to the pool before any deposit
    deal(USDC_TOKEN_ARB, address(commonPool), arbitrary_amount);
    assertEq(IERC20(USDC_TOKEN_ARB).balanceOf(address(commonPool)), arbitrary_amount);


    // Dealing USDC to address(this), approving, and depooositing into the pool
    deal(USDC_TOKEN_ARB, alice, alice_amount);
    vm.startPrank(alice);
    IERC20(USDC_TOKEN_ARB).approve(address(commonPool), alice_amount);
    commonPool.deposit(alice_amount, alice);
    

    // Warp to next epoch so that nav is updated
    assertEq(commonPool.totalAssets(), 0); // Assets are 0 even though we deposited because we need to wait for the NAV update through swapExcessAndRecalculate
    vm.warp(block.timestamp + commonPool.EPOCH_DURATION() + 1);
    uint256 lastEpoch = commonPool.currentEpoch();
    commonPool.endCurrentEpoch();
    uint256 currentEpoch = commonPool.currentEpoch();
    uint256 nextEpoch = currentEpoch + 1;
    console2.log(""[+] Epoch ended after the EPOCH_DURATION"");
    vm.startPrank(address(this));
    _setPoolValue(commonPool, arbitrary_amount + alice_amount, false);
    console2.log(commonPool.totalAssets());
    vm.startPrank(alice);

    // Dealing USDC to address(this), approving, and depooositing into the pool
    deal(USDC_TOKEN_ARB, alice, alice_amount);
    vm.startPrank(alice);
    IERC20(USDC_TOKEN_ARB).approve(address(commonPool), alice_amount);
    commonPool.deposit(alice_amount, alice);

    // Ensuring outcomes
    assertEq(IERC20(USDC_TOKEN_ARB).balanceOf(address(commonPool)), 3000e6);
    assertEq(IERC20(USDC_TOKEN_ARB).balanceOf(alice), 0);
    assertEq(commonPool.balanceOf(alice), 1500e6);

    commonPool.requestWithdraw(1500e6, alice);

    // Warp to next epoch so that nav is updated
    assertEq(commonPool.totalAssets(), 2000e6);
    vm.warp(block.timestamp + commonPool.EPOCH_DURATION() + 1);
    commonPool.endCurrentEpoch();
    vm.startPrank(address(this));
    _setPoolValue(commonPool, arbitrary_amount + 2 * alice_amount, false);
    assertEq(commonPool.totalAssets(), 3000e6);
    console2.log(commonPool.totalAssets());
    vm.startPrank(alice);

    // Alice withdraw all her shares
    commonPool.withdraw(3000e6, alice, alice); // This fails because of a divide by 0 error in the withdraw function due to scaleVariables
}

It is recommended to fix the _scaleVariables function in order to allow complete withdrawal of all shares.

SOLVED: the RFX Exchange team solved this issue by separately handling the case where finalSupply is zero.

Anyone is able to call endEpoch function, once the EPOCH_DURATION is over, triggering the next epoch start. The consequence is that all programmed withdrawals will now be enabled, whether there is or not sufficient liquidity on the contract. Users may or may not be able to withdraw in that case.

There is no access control on that function, neither guarantees that the contract has enough liquidity to allow all approved withdrawals:

function endCurrentEpoch() external {
    if (block.timestamp <= lastEpochUpdate + EPOCH_DURATION) {
        revert IErrors.EpochInProg();
    }
    _endCurrentEpoch();
}

It is recommended to restrict access to the function or to assert sufficient liquidity on the contract to allow increasing the epoch.

SOLVED: the RFX Exchange team solved this issue by removing the epoch feature.

The swapExcessAndRecalculate function in the CommonPool contract accepts Ether, designed to update the Pyth oracle and perform a swap action. However, any excess Ether value that is not used for these operations is not refunded to the sender. Instead, this excess value becomes stuck in the SwapModule contract.

The SwapModule contract also does not verify that the sent Ether value is exactly equal to the required sendWnt amount. This lack of verification adds up to the problem of excess Ether retention.

The following snippet shows the implementation swapExcessAndRecalculate that does not feature an Ether refund:

function swapExcessAndRecalculate(
    bytes[] memory _swapInput,
    bytes[] calldata _pythUpdateData,
    bool _endEpoch,
    IOracle.SetPricesParams memory _oracleParams
) public payable onlyOwner withOraclePrices(_oracleParams) {
    // Updating pyth price for USDC
    uint256 updateFee = pyth.getUpdateFee(_pythUpdateData);
    pyth.updatePriceFeeds{value: updateFee}(_pythUpdateData);

    if (_swapInput.length > 0) {
        // Approving the tokens and slicing the input bytes linked to approval
        _swapInput = _approveSpending(_swapInput);
        // Swapping via the swapModule
        swapModule.swap{value: msg.value - updateFee}(_swapInput);
    }

    // Updating the accounting - navValue is in the deposit token
    address depositAsset = address(asset);
    (uint256 _navValue, address[] memory markets, uint256[] memory marketBalances) =
        helper.calculateNAV(0, depositAsset, decimals, oracleId[depositAsset], activeMarkets.values());

    // Removing empty markets
    _removeMarkets(markets, marketBalances);

    // Updating the stored navValule and distributing NAV per share
    navStored = uint128(_navValue);
    uint256 newShareToAssetPrice = (_navValue * PRECISION) / totalSupply;
    shareToAssetPrice = newShareToAssetPrice;
    shareToAssetPriceUsed = newShareToAssetPrice;

    if (_endEpoch) _endCurrentEpoch();

    emit ReallocationEnded(currentEpoch, _navValue, shareToAssetPrice);
}

It is recommended to refund any excess ETH or prevent the caller to send a different value than required.

SOLVED: the RFX Exchange team solved this issue by refunding the excess ether.

The current deduplication algorithm in the contract has a time complexity of O(n²), which can be optimized to O(n) for better performance. The existing algorithm iterates through the entire list for each item, leading to inefficient processing for larger datasets.

Here is the current implementation:

function _removeDuplicateMarkets(address[] memory _marketList) internal pure returns (address[] memory) {
    // Creating a new array to store the final list - array could be too long if duplicates
    address[] memory _finalList = new address[](_marketList.length);
    uint256 uniqueItems;

    // Adds unique items to the final list
    for (uint256 i = 0; i < _marketList.length;) {
        address currentOuterItem = _marketList[i];
        for (uint256 j = 0; j < _finalList.length; j++) {
            address currentFinalItem = _finalList[j];
            if (currentOuterItem == currentFinalItem) break;
            if (j == _finalList.length - 1) {
                _finalList[uniqueItems] = currentOuterItem;
                uniqueItems++;
            }
        }
        unchecked {
            i++;
        }
    }

    retur

It is recommended to implement a mapping-based approach:

1. Use a mapping for quick lookup.

2. If an item is not in the mapping, add it to both the mapping and the final array.

3. Clear the mapping after use to maintain clean state.

Since mappings are state variables, ensure to clear the mapping after use to maintain a clean contract state.

SOLVED: the RFX Exchange team solved this issue by removing the deduplication element completely.

In the _approveAllocation implementation of the CommonPool contract, when a token is included in both withdrawTx and in the depositTx, the amount to approve in the deposit will overwrite the one in withdraw, instead of adding up. Likewise, if a token is included more than once inside one deposit/withdraw multicall, the allowance will not be increased. That may cause problems when using the same token in a withdrawal and a deposit in the same transaction.

function _approveAllocation(WithdrawTx calldata _withdraw, DepositTx calldata _deposit) internal {
    // Approving the spending from withdrawTx
    for (uint256 i; i < _withdraw.multicall.position.length;) {
        IERC20(_withdraw.multicall.sendToken[i].token).approve(
            routerSender, _withdraw.multicall.sendToken[i].amount
        );
        unchecked {
            i++;
        }
    }

    // Approving the spending from depositTx
    for (uint256 i = 0; i < _deposit.multicall.position.length;) {
        IERC20(_deposit.multicall.sendToken[i].token).approve(
            routerSender, _deposit.multicall.sendToken[i].amount
        );
        unchecked {
            i++;
        }
    }
}

It is recommended to track a list of required allowances so that there can only be one approve call per token.

SOLVED: the RFX Exchange team solved this issue by aggregating the allowances in a requiredAllowances variable.

There is a potential reentrancy in the allocateFunds function, because there is a foreign call (ERC20 approve) on a user input (markets). However, the user is an administrator and is not likely to use a malicious contract address as an input, and legitimate markets are RFX pools, also not likely to be malicious.

The consequence of such a reentrancy would likely be repeating a multicall deposit or withdraw that would have lot of chances to fail and with not much impact, but could still be part of a bigger exploit involving third parties like RFX exchange contracts.

The following snippet shows that the orderNonce variable is incremented after the foreign call:

function allocateFunds(WithdrawTx calldata _withdraw, DepositTx calldata _deposit) external payable {
    // Checking reports are valid and signed and building the transaction data for the multicall
    (bytes[] memory _data, address[] memory _markets) = helper.checkReportsAndBuild(_withdraw, _deposit, orderNonce);

    // Approving funds being sent via SendTokens
    _approveAllocation(_withdraw, _deposit); <--------- foreign call here

    // Updating enumerable set with the new markets
    _addMarkets(_markets);

    // Send transaction data to the router
    orderNonce++; // <------- updated after the foreign call
    router.multicall{value: msg.value}(_data);
    emit ReallocationInitiated(_withdraw, _deposit);
}

function _approveAllocation(WithdrawTx calldata _withdraw, DepositTx calldata _deposit) internal {
    // Approving the spending from withdrawTx
    for (uint256 i; i < _withdraw.multicall.position.length;) {
        IERC20(_withdraw.multicall.sendToken[i].token).approve(
            routerSender, _withdraw.multicall.sendToken[i].amount
        );
        unchecked {
            i++;
        }
    }

    // Approving the spending from depositTx
    for (uint256 i = 0; i < _deposit.multicall.position.length;) {
        IERC20(_deposit.multicall.sendToken[i].token).approve(
            routerSender, _deposit.multicall.sendToken[i].amount
        );
        unchecked {
            i++;
        }
    }
}

It is recommended to update state variables participating to assertions (such as orderNonce) before the foreign call.

SOLVED: the RFX Exchange team solved this issue by adding access control on the vulnerable reentrant function.

It was found that addresses parameter were not totally validated not to be equal to zero, that could lead to failure of business logic if that happen:

• CommonPool.sol→updateApprovedExternalAddr

• CommonPool.sol→updateOracleId

• swaps/SwapModule.sol→setModuleAddr

For example, in updateOracleId:

function updateOracleId(address[] calldata _token, bytes32[] calldata _oracleId) external onlyOwner {
    if (_token.length != _oracleId.length) revert IErrors.InvalidLength();
    for (uint256 i; i < _token.length;) {
        oracleId[_token[i]] = _oracleId[i];
        unchecked {
            i++;
        }
    }

    emit OracleIdUpdated(_token, _oracleId);
}

It is recommended to validate that addresses cannot be the zero address before updating variables.

SOLVED: the RFX Exchange team solved this issue by adding the missing zero checks.

It was found that the SwapModule contract does not emit events when updating its variables (setModuleAddr, setApprovedCaller), and that the CommonPool's updateProtocolAddresses function may update the _eventEmitter but does not reflect it in the emitted event.

For example, with the setModuleAddr:

function setModuleAddr(uint256[] calldata _id, address[] calldata _addr) public onlyOwner {
    if (_id.length != _addr.length) revert IErrors.InvalidLength();
    for (uint256 i = 0; i < _id.length;) {
        moduleAddr[_id[i]] = _addr[i];
        unchecked {
            i++;
        }
    }
}

It is recommended to emit events with all the relevant information.

SOLVED: the RFX Exchange team solved this issue by adding the missing events.

The share to asset price is the ratio between the pool net value and the amount of emitted shares. When a user deposits, the new calculation does not take the minted shares into account:

function _scaleVariables(uint256 shares, bool isDeposit) internal {
    uint256 supply = totalSupply;
    shareToAssetPrice = (shareToAssetPrice * supply) / (isDeposit ? supply + shares : supply - shares);
}

For example, a pool of 1000 shares for 1000 USD net value has a price of 1 USD per share. If Alice deposits 1000 USD, she will receive 1000 shares. The share to asset price would therefore be 2000 USD for 2000 shares, 1 USD per share. However, with the current calculations, it is computed to be 1000 USD for 2000, 0.5 USD per share.

The share to asset parameter is not used anywhere else than in that computation, so the finding is informational.

It is recommended to correct the calculations behind the shareToAssetPrice.

SOLVED: the RFX Exchange team solved this issue by fixing the math behind the shareToAssetPrice ratio.