Centrifuge - 0xmystery's results

Inconsistency of checkTransferRestriction() implementation

Unless it's being intended for, checkTransferRestriction in InvestmentManager.sol has not been fairly imposed on all pairs of deposit/redeem logic.

Here are the two instances entailed:

1. LiquidityPoolLike(liquidityPool).checkTransferRestriction is enforced in handleExecutedDecreaseRedeemOrder() but not in handleExecutedDecreaseInvestOrder().
2. lPool.checkTransferRestriction is enforced in _deposit() but not in _redeem().

Gas poisoning on optional logic

In InvestmentManager.sol, if an amount of 0 is passed, this triggers cancelling outstanding deposit/redemption orders. Since no amount of currency or tranche tokens have been involved, it does not make good sense trying to send cancelInvestOrder() or cancelRedeemOrder() through the gateway. Apparently, a malicious actor could send lots of requests entailing 0 amounts, thereby congesting the gateway.

Consider refactoring the affected logic:

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L127-L133

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

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L159-L165

        if (_trancheTokenAmount == 0) {
-            // Case: outstanding redemption orders will be cancelled
-            gateway.cancelRedeemOrder(
-                lPool.poolId(), lPool.trancheId(), user, poolManager.currencyAddressToId(lPool.asset())
-            );
            return;
        }

Excessive calls on _updateLiquidityPoolPrice()

In InvestmentManager.sol, each time handleExecutedCollectInvest() or handleExecutedCollectRedeem() is called, _updateLiquidityPoolPrice() will be invoked to update the liquidity pool with the latest tranche token price. This could be add up excessively considering the amount of requests executed usually ran in huge volumes.

Since updateTrancheTokenPrice() has already been implemented to accomplish the same purpose at the end of each epoch, consider removing the optional code logic:

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L252 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L274

-        _updateLiquidityPoolPrice(liquidityPool, currencyPayout, trancheTokensPayout);

Unneeded modifier

LiquidityPool.withApproval is practically not needed.

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L97-L100

    modifier withApproval(address owner) {
        require(msg.sender == owner, "LiquidityPool/no-approval");
        _;
    }

For instance, withdraw() below may simply be refactored by removing the withApproval() visibility:

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L176-L184

-    function withdraw(uint256 assets, address receiver, address owner)
+    function withdraw(uint256 assets, address receiver)
        public
-        withApproval(owner)
        returns (uint256 shares)
    {
-        uint256 sharesRedeemed = investmentManager.processWithdraw(assets, receiver, owner);
+        uint256 sharesRedeemed = investmentManager.processWithdraw(assets, receiver, msg.sender);
        emit Withdraw(address(this), receiver, owner, assets, sharesRedeemed);
        return sharesRedeemed;
    }

All other instances entailed: https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L200-L208 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L214-L217 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L231-L234 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L247-L249 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L253-L255

Uninitialized variables

Consider initializing essential state variables at the constructor to ensure smooth early function calls.

For instance, gateway and poolManager in InvestmentManager.sol should be initialized upon contract deployment just as this has been implemented LiquidityPool.sol, Gateway.sol and Root.sol rather than strictly through file methods.

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L88-L94

    constructor(address escrow_, address userEscrow_) {
        escrow = EscrowLike(escrow_);
        userEscrow = UserEscrowLike(userEscrow_);
+        gateway = GatewayLike(data);
+        poolManager = PoolManagerLike(data);

        wards[msg.sender] = 1;
        emit Rely(msg.sender);
    }

All other instances entailed: https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/PoolManager.sol#L120-L125 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/gateway/routers/axelar/Router.sol#L62-L70 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L83-L88 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/Tranche.sol#L42-L46

Incorrect comments

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L140

-    ///         maxDeposit is the max amount of shares that can be collected.
+    ///         maxDeposit is the max amount of assets that can be deposited.

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/PoolManager.sol#L65

-    // important: the decimals of the leading pool currency.
+    // important: the decimals of the lending pool currency.

Cached variable not fully utilized

In InvestmentManager.requestDeposit, lPool.asset() has been cached as currency but it is still being used in subsequent code lines:

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L117-L141

    function requestDeposit(uint256 currencyAmount, address user) public auth {
        address liquidityPool = msg.sender;
        LiquidityPoolLike lPool = LiquidityPoolLike(liquidityPool);
        address currency = lPool.asset();
        uint128 _currencyAmount = _toUint128(currencyAmount);

        // Check if liquidity pool currency is supported by the Centrifuge pool
        poolManager.isAllowedAsPoolCurrency(lPool.poolId(), currency);
        // Check if user is allowed to hold the restricted tranche tokens
        _isAllowedToInvest(lPool.poolId(), lPool.trancheId(), currency, user);
        if (_currencyAmount == 0) {
            // Case: outstanding investment orders only needed to be cancelled
            gateway.cancelInvestOrder(
-                lPool.poolId(), lPool.trancheId(), user, poolManager.currencyAddressToId(lPool.asset())
+                lPool.poolId(), lPool.trancheId(), user, poolManager.currencyAddressToId(currency)
            );
            return;
        }

        // Transfer the currency amount from user to escrow. (lock currency in escrow).
        SafeTransferLib.safeTransferFrom(currency, user, address(escrow), _currencyAmount);

        gateway.increaseInvestOrder(
-            lPool.poolId(), lPool.trancheId(), user, poolManager.currencyAddressToId(lPool.asset()), _currencyAmount
+            lPool.poolId(), lPool.trancheId(), user, poolManager.currencyAddressToId(currency), _currencyAmount
        );
    }

Membership expiration

All investors have been KYC'd and AML'd before they could deposit/mint and withdraw/redeem in LiquidityPool.sol. Under this context, consider making their membership indefinite. Gas saving on reduced logic aside, investors would not have to run into issues where their requests and executions (processes) got denied just moments after the membership had expired. Additionally, wards would not have to take the trouble to periodically extend investors' expired membership. And if an investor needed to be removed, the option would always be there for the ward to disfellowship it.

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/RestrictionManager.sol#L57-L67

-    function updateMember(address user, uint256 validUntil) public auth {
+    function updateMember(address user) public auth {
-        require(block.timestamp <= validUntil, "RestrictionManager/invalid-valid-until");
-        members[user] = validUntil;
+        members[user] = type(uint256).max;
    }

-    function updateMembers(address[] memory users, uint256 validUntil) public auth {
+    function updateMembers(address[] memory users) public auth {
        uint256 userLength = users.length;
        for (uint256 i = 0; i < userLength; i++) {
-            updateMember(users[i], validUntil);
+            updateMember(users[i]);
        }
    }

Inexpedient previews

In InvestmentManager.sol, previewDeposit(), previewMint(), previewWithdraw() and previewRedeem() will all be returning 0 if depositPrice/redeemPrice == 0. Additionally, the prices will be pre-determined from lpValues.maxDeposit/lpValues.maxMint or lpValues.maxWithdraw/lpValues.maxRedeem via calculateDepositPrice() or calculateRedeemPrice(). Like it or not, these static prices aren't going to change regardless of the latest prices till the processes are called.

Misleading variable name

Consider renaming the memory variable below to enhance readability and avoid confusion.

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L591-L603

    function _calculateTrancheTokenAmount(uint128 currencyAmount, address liquidityPool, uint256 price)
        internal
        view
        returns (uint128 trancheTokenAmount)
    {
        (uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);

-        uint256 currencyAmountInPriceDecimals = _toPriceDecimals(currencyAmount, currencyDecimals, liquidityPool).mulDiv(
+        uint256 trancheTokenAmountInPriceDecimals = _toPriceDecimals(currencyAmount, currencyDecimals, liquidityPool).mulDiv(
            10 ** PRICE_DECIMALS, price, MathLib.Rounding.Down
        );

-        trancheTokenAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, trancheTokenDecimals, liquidityPool);
+        trancheTokenAmount = _fromPriceDecimals(trancheTokenAmountInPriceDecimals, trancheTokenDecimals, liquidityPool);
    }

Functions line separation within contracts

Functions within contracts should be separated by a single line as denoted in the link below:

https://docs.soliditylang.org/en/latest/style-guide.html#blank-lines

The bot report brought up this issue albeit with wrong instances entailed. Here's one of the supposed instances:

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L9-L42

interface ERC20PermitLike {
    function permit(address owner, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
        external;
    function PERMIT_TYPEHASH() external view returns (bytes32);
    function DOMAIN_SEPARATOR() external view returns (bytes32);
}

interface TrancheTokenLike is IERC20, ERC20PermitLike {
    function checkTransferRestriction(address from, address to, uint256 value) external view returns (bool);
}

interface InvestmentManagerLike {
    function processDeposit(address receiver, uint256 assets) external returns (uint256);
    function processMint(address receiver, uint256 shares) external returns (uint256);
    function processWithdraw(uint256 assets, address receiver, address owner) external returns (uint256);
    function processRedeem(uint256 shares, address receiver, address owner) external returns (uint256);
    function maxDeposit(address user, address _tranche) external view returns (uint256);
    function maxMint(address user, address _tranche) external view returns (uint256);
    function maxWithdraw(address user, address _tranche) external view returns (uint256);
    function maxRedeem(address user, address _tranche) external view returns (uint256);
    function totalAssets(uint256 totalSupply, address liquidityPool) external view returns (uint256);
    function convertToShares(uint256 assets, address liquidityPool) external view returns (uint256);
    function convertToAssets(uint256 shares, address liquidityPool) external view returns (uint256);
    function previewDeposit(address user, address liquidityPool, uint256 assets) external view returns (uint256);
    function previewMint(address user, address liquidityPool, uint256 shares) external view returns (uint256);
    function previewWithdraw(address user, address liquidityPool, uint256 assets) external view returns (uint256);
    function previewRedeem(address user, address liquidityPool, uint256 shares) external view returns (uint256);
    function requestRedeem(uint256 shares, address receiver) external;
    function requestDeposit(uint256 assets, address receiver) external;
    function collectDeposit(address receiver) external;
    function collectRedeem(address receiver) external;
    function decreaseDepositRequest(uint256 assets, address receiver) external;
    function decreaseRedeemRequest(uint256 shares, address receiver) external;
}

All other instances mostly hover around the in file interfaces: https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L8-L56 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/RestrictionManager.sol#L6-L10 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/Escrow.sol#L7-L9 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/PoolManager.sol#L11-L50

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.21",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Kindly visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here is one particular example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

This report provides a comprehensive analysis of the Centrifuge project's smart contract codebase, focusing on potential vulnerabilities, architecture recommendations, codebase quality, centralization, mechanism intricacies, and systemic risks. Drawing on a meticulous evaluation of the code and in-depth understanding of the project's objectives, the report aims to present findings that can enhance the security and efficiency of the protocol, ensuring its robustness in the ever-evolving DeFi landscape. Let's delve into the details of each aspect, starting with specific findings and overarching recommendations.

Comments for the judge to contextualize my findings

I submitted a high and a medium findings, and herewith on their breakdown in the summary below:

1. Incorrect Approval of Contracts in PoolManager.deployLiquidityPool (High): The deployLiquidityPool function in the PoolManager smart contract contains a critical bug in which it mistakenly tries to grant the investmentManager and liquidityPool contracts permission to spend a non-token contract (liquidityPool itself) instead of the intended tranche.token. Such an error not only fails to accomplish the desired action but can cause transaction reverts when certain functions, like handleExecutedDecreaseRedeemOrder(), are invoked. The source of this error lies in the wrong usage of the first argument for the approve function in lines 328 and 329 of the deployLiquidityPool function, where liquidityPool is erroneously used instead of tranche.token. Although both contract and token addresses are of the same type, causing no compilation error, the actual functionality breaks down. The recommended fix is to correct the first argument in the mentioned approve function calls to use tranche.token.
2. No slippage protections on deposit/mint and withdraw/redeem (Medium): The LiquidityPool contract's convertToShares and convertToAssets functions depend on token prices retrieved from the latest Centrifuge epoch. Despite the general expectation that investors deposit early and withdraw late due to rising prices, the reality is that the Centrifuge pool's Net Asset Value (NAV) can suffer losses, particularly from defaults. It doesn't always align with the expected growth trend driven by borrower interest. A cited scenario demonstrates how a user could be negatively impacted due to sharp drops in Centrifuge's returned prices, leading to significant financial losses. To address this vulnerability, it's recommended to introduce slippage protections for the requestDeposit() and requestRedeem() functions. This way, transactions won't execute if Centrifuge's returned prices fall outside the investors' preferred ranges. Investors will then have the choice to await the next execution epoch or cancel their requests.

Approach taken in evaluating the codebase

As always, I started off by reading the docs and links provided to help gain a good understanding on the accounting and business intentions of the protocol. Covering both the in scope and the out of scope sections are necessary to help piece together all puzzles relating to the specific contracts under audit in this contest. I agree with the general consensus that the codebase is very well structured, making it near to impossible to detect any critical vulnerabilities. Nonetheless, it is intriguing knowing that my high school math and financial experiences could be put to such a practical use in smart contract auditing that I have never imagined. Certainly there is a time for everything regardless of the contest results. That said, paying attention to what the sponsor has explained and highlighted in the Discord Channel is also of paramount importance to focus on the steered direction.

Architecture recommendations

The inconsistent implementation of the checkTransferRestriction() function across deposit/redeem logics, the potential for gas poisoning through optional logic, excessive calls to _updateLiquidityPoolPrice(), the redundancy of the LiquidityPool.withApproval modifier, uninitialized essential state variables, inaccurate comments, underutilized cached variables, issues with membership expiration, misleading variable names, and improper function line separations according to Solidity's style guide are some of the key issues addressed in the QA report.

On top of that, I suggest having the contracts under audit equipped with a complete set of NatSpec detailing all input and output parameters pertaining to each functions although much has been done in specifying each @notice.

Codebase quality analysis

I would say the Centrifuge protocol showcases a highly commendable codebase, characterized by its exceptional readability, comprehensive documentation, and an emphasis on modularity. Its code is clearly articulated, adopting standardized naming conventions for easy comprehension and efficient debugging. An in-depth explanation is provided for all classes, methods, and data structures, aiding developers in understanding the protocol's intricacies. The codebase is segmented into distinct modules, each responsible for a specific functionality, promoting reusability and scalability. A robust testing suite coupled with a well-configured continuous integration pipeline ensures code correctness and timely bug detection. The rigorous code review process and regular security audits add another layer of reliability, while the detailed audit trails offer a transparent view of all modifications, ensuring a high-quality, secure, and efficient protocol.

Centralization risks

There seems to be a good mix of intended centrality and governance ensuring that the system will not be rug pulled and yet fortified with pausable emergency routes. A call for a heightened level of community responsibility and meticulous scrutiny to ensure the protocol's integrity and resilience against potential vulnerabilities would perhaps underscore the importance of strong community engagement and the adherence to comprehensive security measures. Doing this will greatly foster users' sense of ownership and widely expand the horizon of the community base in a mutual sense.

Mechanism review

In evaluating the underlying mechanisms of the Centrifuge protocol, it is evident that it employs a sophisticated blend of advanced financial strategies and cutting-edge blockchain technology. The design focuses on achieving optimal balance between high-yield returns for investors and ensuring the safety and security of assets. However, it's crucial to acknowledge that all mechanisms, regardless of their complexity, may harbor potential pitfalls. As with any DeFi project, the intricate interplay of multiple components can introduce vectors of vulnerability or inefficiency. Thus, a continuous review and iterative improvement of these mechanisms are essential, adapting to real-world challenges and feedback to ensure the protocol remains resilient and advantageous for its users.

Systemic risks

Inherent systemic risks cannot be completely eliminated in the current setup, particularly considering that a significant portion of the proposed designs are still in the experimental phase and haven't yet been deployed live. The undeployed designs could bring unforeseen challenges and issues to light upon activation, potentially affecting the stability and security of the protocol. One possible measure to mitigate these risks could be increasing the implementation of distributed governance structures, a Decentralized Autonomous Organization (DAO) the protocol has been fostering via its CFG, which can bring a layer of resilience and adaptive management to the system. DAOs have the potential to diffuse systemic risks by providing a platform for collective decision-making and fast, effective responses to unexpected threats or issues. However, the transition to a stronger DAO must be carefully managed, as it introduces its own set of complexities and demands strong community engagement and active participation.

I also think continued efforts in getting additional mitigation reviews are of paramount importance to better safeguard the first of its kind deFi logic both in terms of atomicity and the gas reduced transactions.

Time spent:

30 hours