# LIBRE Audit Findings - Vishnuram

## 01. [High] Admin cancellation of first order in queue will lead to failing of settling orders

**File(s)**: [`SubscriptionBook.sol`](https://github.com/NethermindEth/libre-platform-contracts/blob/e88db8da563a0acbe3bffa7af911714215759d4c/src/SubscriptionBook.sol)

**Description**: Admin has privilege of cancelling order even after the tokens are locked for a particular order. In that case the orderId is removed from the queue. But in a special case after an order is cancelled, settle order functionality fails.

function settleOrders(uint256 _lastOrderId, uint256 _percentageToSettle, bytes32 _role) external override {
    ...
    if (_lastOrderId < nextOrderToSettle) {
        revert IOrderPipelineOrderAlreadySettledInCurrentRound();
    }

    uint256[] memory allOrders = getIdsOrderFrom(nextOrderToSettle == 0 ? start : nextOrderToSettle, _lastOrderId);
    ...
    nextOrderToSettle = idsQueue[_lastOrderId].next;
}

Let's take a scenario were there are five orders(1,2,3,4,5)
1. Admin first settles orders till 2. Queue is updated and `nextOrderToSettle` is updated to 3.
2. Later admin cancels the order 3 which updates the `start` variable in `idSorter` contract to 4 and orders queue look like (1,2,4,5).
3. Then admin wants to settle orders till 5, which should basically settle order 4 and 5. But since `nextOrderToSettle` is not updated, `settleOrders` function will fetch for orders from 3 to 5 which reverts because of `IdSorterToParamNotFound()` error.

function testSettleOrders() public {
    uint256 _amount = 1000;
    instrumentRegistry.updateAuditedNavPerShare(
        keccak256("LIBRE_FUND_ADMIN_ROLE"), subscriptionBook.instrumentId(), 1000
    );
    dealerRegistry.allowDealer(0x0, LIBRE_DEALER, LIBRE_INSTRUMENT, true);
    investorRegistry.allowInvestor(LIBRE_DEALER_ROLE, LIBRE_INVESTOR);
    paymentToken.mint(LIBRE_INVESTOR_WALLET, _amount); // 1000
    dealerRegistry.allowDealer(0x0, LIBRE_DEALER, LIBRE_INSTRUMENT, true);
    investorRegistry.allowInvestor(LIBRE_DEALER_ROLE, LIBRE_INVESTOR_2);
    paymentToken.mint(LIBRE_INVESTOR_WALLET_2, _amount + 1000); // 2000
    dealerRegistry.allowDealer(0x0, LIBRE_DEALER, LIBRE_INSTRUMENT, true);
    investorRegistry.allowInvestor(LIBRE_DEALER_ROLE, LIBRE_INVESTOR_3);
    paymentToken.mint(LIBRE_INVESTOR_WALLET_3, _amount + 100); // 1100
    dealerRegistry.allowDealer(0x0, LIBRE_DEALER, LIBRE_INSTRUMENT, true);
    investorRegistry.allowInvestor(LIBRE_DEALER_ROLE, LIBRE_INVESTOR_4);
    paymentToken.mint(LIBRE_INVESTOR_WALLET_4, _amount + 200); // 1200
    dealerRegistry.allowDealer(0x0, LIBRE_DEALER, LIBRE_INSTRUMENT, true);
    investorRegistry.allowInvestor(LIBRE_DEALER_ROLE, LIBRE_INVESTOR_5);
    paymentToken.mint(LIBRE_INVESTOR_WALLET_5, _amount - 100); // 900

    // five investors create order, confirm and lock tokens
    vm.startPrank(LIBRE_INVESTOR_WALLET);
    uint256 orderId1 = subscriptionBook.investorCreateOrder(_amount);
    subscriptionBook.investorConfirmOrder(orderId1);
    paymentToken.approve(address(subscriptionBook), type(uint256).max);
    subscriptionBook.investorLockTokens(orderId1);
    vm.stopPrank();

    vm.startPrank(LIBRE_INVESTOR_WALLET_2);
    uint256 orderId2 = subscriptionBook.investorCreateOrder(_amount + 1000);
    subscriptionBook.investorConfirmOrder(orderId2);
    paymentToken.approve(address(subscriptionBook), type(uint256).max);
    subscriptionBook.investorLockTokens(orderId2);
    vm.stopPrank();

    vm.startPrank(LIBRE_INVESTOR_WALLET_3);
    uint256 orderId3 = subscriptionBook.investorCreateOrder(_amount + 100);
    subscriptionBook.investorConfirmOrder(orderId3);
    paymentToken.approve(address(subscriptionBook), type(uint256).max);
    subscriptionBook.investorLockTokens(orderId3);
    vm.stopPrank();

    vm.startPrank(LIBRE_INVESTOR_WALLET_4);
    uint256 orderId4 = subscriptionBook.investorCreateOrder(_amount + 200);
    subscriptionBook.investorConfirmOrder(orderId4);
    paymentToken.approve(address(subscriptionBook), type(uint256).max);
    subscriptionBook.investorLockTokens(orderId4);
    vm.stopPrank();

    vm.startPrank(LIBRE_INVESTOR_WALLET_5);
    uint256 orderId5 = subscriptionBook.investorCreateOrder(_amount - 100);
    subscriptionBook.investorConfirmOrder(orderId5);
    paymentToken.approve(address(subscriptionBook), type(uint256).max);
    subscriptionBook.investorLockTokens(orderId5);
    vm.stopPrank();

    subscriptionBook.settleOrders(orderId2, 10000, keccak256("LIBRE_FUND_ADMIN_ROLE")); // admin settling orders till 2
    subscriptionBook.adminCancelOrder(orderId3, keccak256("LIBRE_FUND_ADMIN_ROLE")); // admin cancelling order 3
    subscriptionBook.settleOrders(orderId5, 10000, keccak256("LIBRE_FUND_ADMIN_ROLE")); // admin trying to settle orders till 5 but reverts
}

While cancelling the order nextOrderToSettle is not updated.

function _afterCancelOrderCheck(uint256 _orderId, Order memory _order, uint256 _submitter) internal {
    ...

    if (_order.confirmed) {
        confirmedAmount -= _order.amount;
        confirmedAmountPerInvestor[_order.investorId] -= _order.amount;

        // If locked update the locked amount
        if (!_order.available) {
            amountLocked -= _order.amount;
            paymentToken.transfer(_order.beneficiary, _order.amount);
            removeId(_orderId);
        }
    }

    ...

    // @audit update the nextOrderToSettle

    // delete the order
    delete orders[_orderId];
    emit OrderCanceled(_orderId, _submitter);
}

**Recommendation(s)**: Consider updating the `nextOrderToSettle` variable while admin cancelling the order to resolve this issue. By setting the variable to next order id in queue if it is equal to order id which is getting cancelled.

**Status**: Unresolved

**Update from the client**:

---

## 02. [Low] Settled order is not marked done
**File(s)**: [`SubscriptionBook.sol`](https://github.com/NethermindEth/libre-platform-contracts/blob/e88db8da563a0acbe3bffa7af911714215759d4c/src/SubscriptionBook.sol)
**Description**: There are four phases in Subscription Book contract which are order creation, order confirmation, locking tokens and settle orders. Two important parameters which track of each phases are `available` and `confirmed`. When order is `confirmed` it is set to `true` and when tokens are locked `available` is set to `false`.

When an order is settled through `_instantSettlement` function, confirmed parameter is set to true correctly which means the order is marked true. But in case of `settleOrders` function this is not happening.

function settleOrders(uint256 _lastOrderId, uint256 _percentageToSettle, bytes32 _role) external override {
    ...
    if (orders[orderId].amount == 0) {
        lastEmptyOrder = orderId;
        amountOfEmptyOrders++;
        _decreaseInvestorOrders(order.investorId);
        // @audit order is not marked as done
    }
    ...
}

**Recommendation(s)**: Consider to mark an settled order as done by setting `false` to `confirmed` parameter, so the Subscription process works as intended. As well as both `_instantSettlement` and `settleOrders` will follow same methodology.

**Status**: Unresolved

**Update from the client**:

---

## 03. [Low] Incorrect definition of `INITIAL_RESTRICTED_PERIOD_ALLOWANCE` constant variable

**File(s)**: [`RedemptionBook.sol`](https://github.com/NethermindEth/libre-platform-contracts/blob/e88db8da563a0acbe3bffa7af911714215759d4c/src/RedemptionBook.sol)

**Description**: Incorrect definition of constant variables can lead to critical issues like reading wrong values from storage.

In RedemptionBook contract `INITIAL_RESTRICTED_PERIOD_ALLOWANCE` is incorrectly defined with the same value of `INITIAL_SUBSCRIPTION_RESTRICTED_PERIOD_ALLOWANCE`. Through this wrong uint value is read to `allowedPercent` variable in `_calculateFeeDeduction` function.

contract RedemptionBook is Initializable, PermissionedContract, IdSorter, IRedemptionBook {
    ...
    bytes32 constant INITIAL_SUBSCRIPTION_RESTRICTED_PERIOD_ALLOWANCE =
        keccak256("INITIAL_SUBSCRIPTION_RESTRICTED_PERIOD_ALLOWANCE");
    bytes32 constant INITIAL_SUBSCRIPTION_RESTRICTED_PERIOD_FEE =
        keccak256("INITIAL_SUBSCRIPTION_RESTRICTED_PERIOD_FEE");
    bytes32 constant INITIAL_RESTRICTED_PERIOD_ALLOWANCE =
        keccak256("INITIAL_SUBSCRIPTION_RESTRICTED_PERIOD_ALLOWANCE"); // @audit value is clashing with INITIAL_SUBSCRIPTION_RESTRICTED_PERIOD_ALLOWANCE
    ...
}

**Recommendation(s)**: Consider changing the value of `INITIAL_RESTRICTED_PERIOD_ALLOWANCE` to `keccak256("INITIAL_RESTRICTED_PERIOD_ALLOWANCE")`

**Status**: Unresolved

**Update from the client**:

---

## 04. [Best Practice] Missing Event Emission

**File(s)**: [`EternalRegistryStorage.sol`](https://github.com/NethermindEth/libre-platform-contracts/blob/e88db8da563a0acbe3bffa7af911714215759d4c/src/EternalRegistryStorage.sol)

**Description**: `_setReservedKey` function in this contract helps to set a reserved key by restricting the key. Several registry contracts use this to set the default reserved keys. It is important to emit an event while this is happening.

function _setReservedKey(bytes32 _id, bytes32 _key) internal {
    internalKeys[_id][_key] = true;
    // @audit missing event emission
}

**Recommendation(s)**: Consider creating a separate event for adding new reserved key, for example `AddedNewReservedKey(bytes32 indexed id, bytes32 indexed key)` and update the `_setReservedKey` function to emit that event.

**Status**: Unresolved

**Update from the client**:

---

## 05. [Best Practice] Presence of unused Variables

**Files**: [`SecurityToken.sol`](https://github.com/NethermindEth/libre-platform-contracts/blob/e88db8da563a0acbe3bffa7af911714215759d4c/src/SecurityToken.sol) [`InstrumentRegistry.sol`](https://github.com/NethermindEth/libre-platform-contracts/blob/e88db8da563a0acbe3bffa7af911714215759d4c/src/registries/InstrumentRegistry.sol)

**Description**: The presence of unused variables is generally considered a practice to be avoided, as they may lead to potential issues such as:
- Increased computational costs (i.e., unnecessary gas consumption);
- Reduced code readability; and
- The possibility of bugs (e.g., when an explicit return statement is forgotten and return values are never assigned).

The state variable `lifecycleContract` in the SecurityToken contract is not used throughout the contract.

contract SecurityToken is ISecurityToken, ERC20Upgradeable {
    ...
    // @audit unused
    address lifecycleContract;
    ...
}

The state variables `subscriptionBookImp` and `redemptionBookImp` in the InstruemtnRegistry contract are not used throughput the contract.

contract InstrumentRegistry is BaseRegistry, IInstrumentRegistry {
    ...
    // @audit unused
    address public subscriptionBookImp;
    address public redemptionBookImp; 
    ...
}

**Recommendation(s)**: We recommend removing all unused variables from the codebase to enhance code quality and minimize the risk of errors or inefficiencies.

**Status**: Unresolved

**Update from the client**:

## 06. [Best Practice] Emitting address instead of index of the wallet

**File(s)**: [`BaseUserRegistry.sol`](https://github.com/NethermindEth/libre-platform-contracts/blob/e88db8da563a0acbe3bffa7af911714215759d4c/src/registries/BaseUserRegistry.sol)

**Description**: `removeWallet` function emits the index of the wallet from the user's wallet address array. But emitting wallet address (`walletToDelete`) will be useful and clear.

function removeWallet(uint256 _index) external override {
    ...
    address walletToDelete = ownedWallets[_userId][_index];
    ...
    emit WalletRemoved(_index); // @audit emit the address of the removed wallet
    ...
}

**Recommendation**: Consider emitting address of wallet removed (`walletToDelete`) in `removeWallet` function. This provides better information for anyone monitoring or interacting with the smart contract.

**Status**: Unresolved

**Update from the client**:

---

## 07. [Best Practice] Verify whether the transfer was successful and the token balance after transfer using order amount

**File(s)**: [`SubscriptionBook.sol`](https://github.com/NethermindEth/libre-platform-contracts/blob/e88db8da563a0acbe3bffa7af911714215759d4c/src/SubscriptionBook.sol)

**Description**: At the locking tokens phase the ERC20 token is transfered from the `sender` to the `SubscriptionBook` contract. In some ERC20 tokens, if the transfer fails it won't revert instead returns false or it won't return anything like in USDT. In that case an order is locked without receiving the tokens.

function _afterLockOrderCheck(uint256 _orderId, Order memory _order, address _investorWallet, uint256 _submitter) internal {
    ...
    paymentToken.transferFrom(sender, address(this), _order.amount); 
    // @audit-issue check the return value
    // @audit-issue verify the token balance before and after the transfer using order amount 
    ...
}

This applies in case of admin cancels an order. Payment tokens are sent to order `beneficiary`

function _afterCancelOrderCheck(uint256 _orderId, Order memory _order, uint256 _submitter) internal {
    ...
    if (_order.confirmed) {
        confirmedAmount -= _order.amount;
        confirmedAmountPerInvestor[_order.investorId] -= _order.amount;

        // If locked update the locked amount
        if (!_order.available) {
            amountLocked -= _order.amount;
            paymentToken.transfer(_order.beneficiary, _order.amount);
            // @audit-issue check the return value
            // @audit-issue verify the token balance before and after the transfer using order amount 
            removeId(_orderId);
        }
    }
    ...
}

**Recommendation(s)**: Consider using SafeERC20 functionalities for transfer and transferFrom. Also, consider comparing the contract's token balances before and after it.