Centrifuge - Kral01's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L160-L162 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L170-L172 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L176-L184 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L148-L152 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L569-L582

Vulnerability details

Impact

As per EIP 4626's Security Considerations

Finally, EIP-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users.

If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.

If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

Thus, the result of the mint, withdraw, previewMint and previewWithdraw should be rounded up.

Proof of Concept

The mint, withdraw, previewMint and previewWithdraw from the LiquidityPool.sol calls thier respective functions in InvestmentManager.sol which ultimately calls the _calculatePrice function. This function rounds_down the prices which is not expected as per the EIP-4626 standard.

//In File::src/InvestmentManager.sol

function _calculatePrice(uint128 currencyAmount, uint128 trancheTokenAmount, address liquidityPool)
        public
        view
        returns (uint256 depositPrice)
    {
        (uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);
        uint256 currencyAmountInPriceDecimals = _toPriceDecimals(currencyAmount, currencyDecimals, liquidityPool);
        uint256 trancheTokenAmountInPriceDecimals =
            _toPriceDecimals(trancheTokenAmount, trancheTokenDecimals, liquidityPool);

        depositPrice = currencyAmountInPriceDecimals.mulDiv(
            10 ** PRICE_DECIMALS, trancheTokenAmountInPriceDecimals, MathLib.Rounding.Down //@audit rounds down
        );
    }

Tools Used

Manual review

Impact

Other users/ protocols that integrate with Centrifuge's Liquidity Pools might wrongly assume that the functions handle rounding as per ERC4626 expectation. Thus, it might cause some integration problem in the future that can lead to wide range of issues for both parties.

Recommended Mitigation Steps

Ensure that the rounding of vault's functions behave as expected. Following are the expected rounding direction for each vault function:

previewMint() - Round Up :arrow_up:

previewWithdraw() - Round Up :arrow_up:

mint() - Round Up :arrow_up:

withdraw() - Round Up :arrow_up:

Other functions are expected to Round down :arrow_down:, which atm is working.

For implementing this, we can use an ENUM

 ENUM Rounding{
 UP,
 DOWN
 } 

Which can be passed to the functions and it can be used to have an if statement to determine whether to round up or down.

Assessed type

Library

1. Analysis of Codebase

The codebase for Centrifuge's Liquidity Pools is well-structured and effectively integrates cross-chain communication. This structure aligns with Centrifuge's mission to bridge businesses or "Asset Originators" and investors through Decentralized Finance (DeFi). Here is a step-by-step process flow of the protocol from a user's perspective:

1. Users initiate a deposit into the protocol via the 'LiquidityPool' contract by sending deposit requests.
2. The 'InvestmentManager' contract handles and validates these requests.
3. The deposit amount is held in escrow by the 'UserEscrow' contract.
4. The deposit requests are encrypted and sent to the Centrifuge chain by the Gateway.sol and messages.sol contracts.
5. The Centrifuge chain processes these requests, and the 'InvestmentManager' contract handles incoming messages.
6. All operations such as deposit, redeem, mint, withdraw etc., follow this execution order.
7. The protocol also provides a feature to decrease your deposit or redeem request while the request is in process.

Access controls are well-implemented using 'root.sol', which serves as the central access control entity for all contracts through a ward feature. The tests are comprehensive and utilize fuzzing, providing a robust coverage. The gateway routers adhere to best practices, and the ERC4626 implementations are mostly compliant with recommended guidelines.

2.Architecture recommendations

1. Consider changing the visibility of functions that are not called by other contracts to internal, as most functions in all contracts are currently public.
2. It might be beneficial to restrict the creation of new pools to members only, even though there aren't immediate threats since tranches and currencies are validated.
3. While the ERC-4626 implementation is mostly correct, there are some missing elements highlighted in the report, and it would be beneficial to address them.
4. Consider conducting a gas audit. Improvements can be made to decrease the gas usage of the codebase, providing a better user experience.

3.Centralization risks

The protocol has a few centralization risks that should be addressed:

1. Admins currently have the ability to pause the Liquidity Pools at any time.
2. The root.sol contract is a central point of control that manages all other contracts and access controls. This presents a risk as any member can be removed at any time. Given that user funds can be locked in the contract if they are removed, this issue should not be overlooked.

4.Time Spent

The audit took a total of 4 days:

1st Day: Understanding the protocol's documentation, process flow, and end user flow, and taking notes and drawing diagrams. 2nd Day: Linking the documentation's logic to the codebase by following the user's entry point, scanning for vulnerabilities by executing the user flow, taking notes of potential vulnerabilities, and reading reports for similar vault-based protocols. 3rd Day: Focusing on different types of attack vectors and documenting found vulnerabilities also reading the tests provided some insight into potential vulnerability and edge cases. 4th Day: Summarizing the audit by completing the QA report and analysis .

Time spent:

48 hours