Centrifuge - Liqudity Pools - Security Review

Author: Manuel - 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

decrease by 20, remaining=80
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.