# Centrifuge - Liqudity Pools - Security Review

Author: Manuel - [xmxanuel](https://twitter.com/xmxanuel)

## 1. Executive Summary
The Centrifuge team has asked me to review their Solidity codbase for `Liqudity Pools` twice as an indepentend security reviewer to give feedback on overall design and to find security vulnerability.

The security reviews took place once during development (`v1`) and a second time with the first version (`v2`).

The following report only includes findings related to security and not the overall design feedback.

### 1.1 Disclaimer
The content of this audit report is provided "as is," without representations and warranties of any kind. The author and his employer disclaim any liability for damage arising out of, or in connection with, this audit report. Copyright of this report remains with the author.

### 1.2 Repository

https://github.com/centrifuge/liquidity-pools

| Version | Commit Hash | Date | Note |
|-------------|-------------------------------------------|---------------|--------------------|
| First review (`v1`)| `e1e1e7caacbb95bb9ad90264b49d0e1ab58e1bb2` | End of June 2023 | Work in Progress Version |
| Second review (`v2`)| `48a7eec230dc0588cbe43b44fea3229717eb9bde` | September 2023 | First Release |

**Excluding from Scope**
The individual router in the `router` directory(`axelar` and `xcm`) were consider out of scope as any other potential attack vectors based on bridges.

In the version `v1` review the correct token price calculations were not implemented.

## Findings

### Summary

| Severity | Findings |
| -------- | -------- |
| HIGH | 5 |
| MEDIUM | 3 |
| LOW | 1 |

### HIGH

#### H1. `Connector._decreaseRedemptionLimits` uses `maxDeposit` instead of `maxWithdraw`
The `_decreaseRedemptionLimits` function decreases the `maxWithdraw` and `maxRedeem`. However, in the following if condition `maxDeposit` is incorrectly set to zero. `

   if (values.maxWithdraw < _currency) {
-            values.maxDeposit = 0;
+            values.maxWithdraw = 0;
        }

The `maxDeposit` function should only be modified in the `_decreaseDepositLimits` and not in the `_decreaseRedemptionLimits`.

#### H2. `LiqudityPool.requestDeposit` uses `auth` modifier instead of `onlyMember`

The `requestDeposit` function uses the `auth` modifier. Which checks if the caller has `admin` rights.

The `requestDeposit` function is indented for `whitelisted` investors. Therefore the `onlyMember` modifier should be used.

    function requestDeposit(uint256 _assets, address _receiver) auth public {

#### H3.v1`LiqudityPool.requestDeposit` should use msg.sender and not a `receiver` parameter

The `requestDeposit` function has a parameter called `_receiver`. The `asset` tokens are transferred from the `_receiver` to the `escrow` contract.

This pattern could be exploited by an other investor. If an investor gave a full approval to the escrow contract, any other investor could trigger the transfer of their tokens to escrow by calling `requestDeposit`.

The `_receiver` parameter should be removed and `msg.sender` should be used instead.

#### H4.v1 `_remainingInvestOrder` parameter in `Connector.handleExecutedCollectInvest` can lead to incorrect `investOrder`

The order of transactions on Ethereum are not guranteed. A user could decrease their invest order twice. This would lead to two seperate bridge on Ethereum.

If the bridge uses different addresses the order of the transactions ins not guranteed.

**Example**
initial order: 100

1. decrease by 20, remaining=80
2. decrease by 20, remaining=60

If 2. is executed before 1. the user will receive 40 back but has a remaining of 80.

Using `delta` change values instead of the `remainingInvestorOrder` could fix this edge case.

The same is true for `handleExecutedCollectRedeem`.

#### H5.v1 LiqudityPool.redeem calls `connector.processWithdraw` with incorrect parameter

The `connector.processWithdraw` function is called with `shares` as first parameter in `redeem`.

However, the `processWithdraw` is expecting `assets` and not `shares`.

### MEDIUM

#### M1.v1 An attacker could front-run `LiquidityPoolFactory.deployLiquidityPool`

An attacker could front-run the `deployLiquidityPool` transaction and use the same params to get the same `salt`. This would block the connector from creating new liquidity pools.

The `salt` calculation could include the `msg.sender` to give each address it's own namespace.

#### M2.v1 Connector.connectorActive modifier check is the wrong direction

    modifier connectorActive() {
        require(gateway.paused(), "CentrifugeConnector/connector-deactivated");

The connector should be active if the gateway is `not` paused. This check is implemented incorrectly.

#### M3.v3 Tranche contract uses `msg.sender` instead of `_msgSender` for the `notRestricted` modifier

The tranche contract uses a trusted forwarder pattern similar for the Liquidity pools. This means the `msg.sender` in liqudity pools is attached to `calldata`.

The tranche contract needs to use consistently only `_msgSender()` instead of `msg.sender`. Which would the the liqudity pool.

This is currently not the case for the `notRestricted` modifier.

    // --- Restrictions ---
    function transfer(address to, uint256 value) public override notRestricted(msg.sender, to, value) returns (bool) {
        return super.transfer(to, value);
    }

### LOW
#### L1.v2 `Root.file` delay can be used to block all `scheduleRely` operations

Consider a upper limit for delay to prevent an incorrect delay value.

#### I1.v1 Informational
A list of information notes and improvements.

**use type(uint).max instead of uint(-1)**
uint256 MAX_UINT256 = type(uint).max;

**No address(this) in events**
It is not needed to emit `address(this)` in events.

**`msg.sender` in constructor shoudn't be a ward**
In most cases the deployement address won't be ward in the system. Instead an additional parameter could be used.

**missing `decreaseInvestOrder` function**
There is currently no `decreaseInvestOrder` function int he system.

**block.timestamp doesn't require a full `uint256`**

A block.timestamp doesn't require a full `uint256` in Gateway.

**Router has function names not in camelCase**
The router has function which are not in camelCase.

**own functions for similar code**
`processDeposit` and `processMint` are very similar the only difference is the parameter `_currencyAmount` vs. `trancheTokenAmount`. The common code could be moved into a seperate fuctnion.

**connector.MAX_UINT256 missing constant**
The `MAX_UINT256` is missing the constant keyword.

### I2.v2 Informational

**Naming Consistency**

LPValues storage lpValues = orderbook[user][liquidityPool];
LPValues storage values = orderbook[user][liquidityPool];

Only use one name within the codebase.

**Lack of using events**

In the Gateway the following function could use events

    // --- Administration ---
    function addIncomingRouter(address router) public auth {
        incomingRouters[router] = true;
    }

    function removeIncomingRouter(address router) public auth {
        incomingRouters[router] = false;
    }

    function setOutgoingRouter(address router) public auth {
        outgoingRouter = RouterLike(router);
    }

**PoolManager pool.isActive**
`isActive` is not used and seems to be legacy code.

**Inconsistent Licenses**
Not all Solidity files use the same licience.

**Missleading Comments**
the comment is about the redemption order and not about invest order.

        if (_currencyAmount == 0) {
            // case: outstanding redemption orders only needed to be cancelled
            gateway.cancelInvestOrder(
                lPool.poolId(), lPool.trancheId(), user, poolManager.currencyAddressToId(lPool.asset())
            );
            return;
        }

**Rename pausable modifier to whenNotPaused**
Following the OZ standard and calling it `whenNotPaused`

### Gas Optimizations

#### G1.v1 Connector struct slot positions
If the slots in the `Tranche` struct would be re-ordered gas could be safed.

    uint64 poolId;
    bytes16 trancheId;
    uint8 decimals;

The following variables could fit into one slot if bundled together.

#### G2.v1 Connector mappings could use one Pool struct

Currently, the `Connector` contract uses multiple mappings with the same `id`.

It would be cheaper to move them into a seperate `Pool` struct which is then indexed by the `id`.

Together with slot position optimization.