LiquidityPool is not fully compliant with ERC4626

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L129-L132 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L154-L157 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L164-L167 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L186-L189 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L349-L367 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L323-L347

Vulnerability details

Impact

The implementations of maxDeposit, maxMint, maxWithdraw, maxRedeem do not take into account additional limits and revert conditions that are present in the corresponding action functions. Therefore the max- functions may overestimate the amount in some cases. This breaks the on-chain composability of ERC4626 vaults.

maxDeposit

According to the ERC-4626 specification,

maxDeposit MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert.

maxDeposit always returns orderbook[user][liquidityPool].maxDeposit.

// File: LiquidityPool.sol
function maxDeposit(address receiver) public view returns (uint256) {
    return investmentManager.maxDeposit(receiver, address(this));
}

// File: InvestmentManager.sol
function maxDeposit(address user, address liquidityPool) public view returns (uint256 currencyAmount) {
    currencyAmount = uint256(orderbook[user][liquidityPool].maxDeposit);
}

However, the implementation of the deposit flow in InvestmentManager.processDeposit has additional revert conditions:

// File: InvestmentManager.sol
    function processDeposit(address user, uint256 currencyAmount) public auth returns (uint256 trancheTokenAmount) {
        address liquidityPool = msg.sender;
        uint128 _currencyAmount = _toUint128(currencyAmount);
        require(
            (_currencyAmount <= orderbook[user][liquidityPool].maxDeposit && _currencyAmount != 0),
            "InvestmentManager/amount-exceeds-deposit-limits"
        );

        uint256 depositPrice = calculateDepositPrice(user, liquidityPool);
        require(depositPrice != 0, "LiquidityPool/deposit-token-price-0");

        uint128 _trancheTokenAmount = _calculateTrancheTokenAmount(_currencyAmount, liquidityPool, depositPrice);
        _deposit(_trancheTokenAmount, _currencyAmount, liquidityPool, user);
        trancheTokenAmount = uint256(_trancheTokenAmount);
    }

    function _deposit(uint128 trancheTokenAmount, uint128 currencyAmount, address liquidityPool, address user)
        internal
    {
        LiquidityPoolLike lPool = LiquidityPoolLike(liquidityPool);

        _decreaseDepositLimits(user, liquidityPool, currencyAmount, trancheTokenAmount); // decrease the possible deposit limits
        require(lPool.checkTransferRestriction(msg.sender, user, 0), "InvestmentManager/trancheTokens-not-a-member");
        require(
            lPool.transferFrom(address(escrow), user, trancheTokenAmount),
            "InvestmentManager/trancheTokens-transfer-failed"
        );

        emit DepositProcessed(liquidityPool, user, currencyAmount);
    }

maxDeposit must return 0 in the cases where executing the deposit would revert with "LiquidityPool/deposit-token-price-0" or "InvestmentManager/trancheTokens-not-a-member".

maxMint, maxWithdraw, maxRedeem

The analysis of maxDeposit applies analogously to maxMint, maxWithdraw and maxRedeem:

• InvestmentManager.processMint has the following revert conditions not handled by maxMint: "LiquidityPool/deposit-token-price-0", "InvestmentManager/trancheTokens-not-a-member".
• InvestmentManager.processWithdraw has the following revert conditions not handled by maxWithdraw: "LiquidityPool/redeem-token-price-0".
• InvestmentManager.processRedeem has the following revert conditions not handled by maxRedeem: "LiquidityPool/redeem-token-price-0".

Additional sources of non-compliance

withdraw MUST support a withdraw flow where the shares are burned from owner directly where msg.sender has EIP-20 approval over the shares of owner. redeem MUST support a redeem flow where the shares are burned from owner directly where msg.sender has EIP-20 approval over the shares of owner.

Due to the fact that the system handles withdraw/redeem action by syndicating them and executing them by a trusted party, these approval flows are not implemented.

convertToShares MUST NOT show any variations depending on the caller. convertToShares MUST NOT revert unless due to integer overflow caused by an unreasonably large input. convertToAssets MUST NOT show any variations depending on the caller. convertToAssets MUST NOT revert unless due to integer overflow caused by an unreasonably large input.

The corresponding functions in InvestmentManager have the auth modifier, meaning that they will revert if LiquidityPool is not a ward of InvestmentManager. This should not be the case if the system is configured correctly, however there is still a theoretical case of non-compliance.

    function convertToShares(uint256 _assets, address liquidityPool) public view auth returns (uint256 shares)
    function convertToAssets(uint256 _shares, address liquidityPool) public view auth returns (uint256 assets)

Tools Used

Manual Review

Recommended Mitigation Steps

The requirements by ERC4626 are there to make vaults composable by default.

The Vault interface is designed to be optimized for integrators with a feature complete yet minimal interface. Details such as accounting and allocation of deposited tokens are intentionally not specified, as Vaults are expected to be treated as black boxes on-chain and inspected off-chain before use.

• Return 0 in the max-function if the additional revert conditions in the process-functions.
• Remove the auth modifier from convertToShares and convertToAssets.

Assessed type

ERC4626

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

maxDeposit, maxMint, maxWithdraw, maxRedeem correlate to values returned by Centrifuge after request executions, NOT the way you perceived.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid

[c4-judge]

gzeon-c4 removed the grade

[gzeon-c4]

does seems to be out of compliance since it is possible to have a nonzero maxDeposit but then removed from the member list

[c4-sponsor]

hieronx (sponsor) confirmed

[gzeon-c4]

Valid but low risk

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b