Centrifuge - lsaudit's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L42-L49 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L67-L77 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L83-L88

Vulnerability details

Impact

Function file() allows to change the name, however, it does not update the domain separator.

The similar issue had been evaluated as Medium during previous Code4rena contests:

• https://github.com/code-423n4/2023-01-reserve-findings/issues/211

and had been evaluated as Medium in other security reviews:

• https://github.com/spearbit/portfolio/blob/master/pdfs/ConnextNxtp-Spearbit-Security-Review.pdf (Medium Risk - 5.2.1 _domainSeparatorV4() not updated after name/symbol change)

Moreover, please notice, that this issue is not the same as my other report: #313 This issue is about not updating domain separator, when name is changed. The root cause of current issue is inside file() - calling file() to update name should also update the domain separator. While the other issue (#313) is related to not correctly initializing domain separator from the beginning. The root cause of that issue is inside constructor() initializing domain separator before initializing name. The mitigation step for the current issue is updating code in file(), while the mitigation step for issue #313 is updating code in constructor().

Proof of Concept

While deploying the contract, constructor is responsible for setting deploymentChainId and _DOMAIN_DEPARATOR:

File: src/token/ERC20.sol

 constructor(uint8 decimals_) {
        decimals = decimals_;
        wards[_msgSender()] = 1;
        emit Rely(_msgSender());

        deploymentChainId = block.chainid;
        _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
    }

The domain separator is calculated using EIP-712 standard. Actually, we can even compare the currently implemented code with OpenZeppelin: https://github.com/fractional-company/contracts/blob/master/src/OpenZeppelin/drafts/EIP712.sol

File: src/token/ERC20.sol

    function _calculateDomainSeparator(uint256 chainId) private view returns (bytes32) {
        return keccak256(
            abi.encode(
                keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
                keccak256(bytes(name)),
                keccak256(bytes(version)),
                chainId,
                address(this)
            )
        );
    }

One of the parameters used to calculate domain separator is name: keccak256(bytes(name)).

The function responsible for returning domain separator looks as below:

    function DOMAIN_SEPARATOR() external view returns (bytes32) {
        return block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid);
    }

Whenever deploymentChainId is not changed - the previously set _DOMAIN_SEPARATOR is being returned (function _calculateDomainSeparator(), which calculates new domain separator is not being called, when deploymentChainId is not changed).

The problem occurs when function file() is called. This function allows to update the name, however, the domain separator won't be updated:

File: src/token/ERC20.sol

    function file(bytes32 what, string memory data) external auth {
        if (what == "name") name = data;
        else if (what == "symbol") symbol = data;
        else revert("ERC20/file-unrecognized-param");
        emit File(what, data);
    }

As it was demonstrated above, domain separator is calculated using EIP-712, which uses the name. However, the domain separator is being updated only when deploymentChainId is changed. This behavior implies, that when name is changed - the domain separator would still be calculated using the old name.

Calling file() to change the name does not update the domain separator.

Tools Used

Manual code review

Recommended Mitigation Steps

Function file() should update the domain separator, whenever it changes the name.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L42-L49

Vulnerability details

Impact

The domain separator is not properly calculated in the constructor - as it does not take into calculation name - which is not yet initialized.

The similar issue had been evaluated as Medium during the previous Code4rena audits:

• https://github.com/code-423n4/2022-05-rubicon-findings/issues/38

Moreover, please notice, that this issue is not the same as my other report: #306 The current issue is about incorrectly setting the domain separator on contract deployment. The root cause of this issue lies inside constructor(), as domain separator is being initialized before name. While the other issue (#306) is about not updating domain separator, when name is changed. The root cause of that issue lies inside file() - whenever we call to change the name, the domain separator should be updated too. The remediation for this issue is to update constructor(), while remediation for issue #306 is to update code inside file().

Proof of Concept

File: src/token/ERC20.sol

    constructor(uint8 decimals_) {
        decimals = decimals_;
        wards[_msgSender()] = 1;
        emit Rely(_msgSender());

        deploymentChainId = block.chainid;
        _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
    }

The constructor() calculates the domain separator, before initializing name.

Tools Used

Manual code review

Recommended Mitigation Steps

Initialize name before calculating domain separator

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L591-L617 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L383-L406

Vulnerability details

Impact

According to documentation - LiquidityPool is: A ERC-4626 compatible contract that enables investors to deposit and withdraw stablecoins to invest in tranches of pools.

The Security Consideration paragraph of EIP-4626 states:

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 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.

However, both previewMint and previewWithdraw rounds down, instead of rounding up, which makes the LiquidityPool.sol non compliant with EIP-4626.

The original implementation for previewMint and previewWithdraw in OpenZeppelin ERC4626 is:

function previewMint(uint256 shares) public view virtual returns (uint256) {
    return _convertToAssets(shares, Math.Rounding.Ceil);
}


function previewWithdraw(uint256 assets) public view virtual returns (uint256) {
    return _convertToShares(assets, Math.Rounding.Ceil);
    }

which suggests that previewMint and previewWithdraw should round up.

Proof of Concept

LiquidityPool.sol defines previewMint and previewWithdraw as follows:

    function previewMint(uint256 shares) external view returns (uint256 assets) {
        assets = investmentManager.previewMint(msg.sender, address(this), shares);
    }

    (...)

    function previewWithdraw(uint256 assets) public view returns (uint256 shares) {
        shares = investmentManager.previewWithdraw(msg.sender, address(this), assets);
    }

Those functions call corresponding methods in InvestmentManager.sol:

 function previewMint(address user, address liquidityPool, uint256 _trancheTokenAmount)
        public
        view
        returns (uint256 currencyAmount)
    {
        uint128 trancheTokenAmount = _toUint128(_trancheTokenAmount);
        uint256 depositPrice = calculateDepositPrice(user, liquidityPool);
        if (depositPrice == 0) return 0;

        currencyAmount = uint256(_calculateCurrencyAmount(trancheTokenAmount, liquidityPool, depositPrice));
    }

    /// @return trancheTokenAmount is type of uin256 to support the EIP4626 Liquidity Pool interface
    function previewWithdraw(address user, address liquidityPool, uint256 _currencyAmount)
        public
        view
        returns (uint256 trancheTokenAmount)
    {
        uint128 currencyAmount = _toUint128(_currencyAmount);
        uint256 redeemPrice = calculateRedeemPrice(user, liquidityPool);
        if (redeemPrice == 0) return 0;

        trancheTokenAmount = uint256(_calculateTrancheTokenAmount(currencyAmount, liquidityPool, redeemPrice));
    }

Those methods calls:

    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(
            10 ** PRICE_DECIMALS, price, MathLib.Rounding.Down
        );

        trancheTokenAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, trancheTokenDecimals, liquidityPool);
    }

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

        uint256 currencyAmountInPriceDecimals = _toPriceDecimals(
            trancheTokenAmount, trancheTokenDecimals, liquidityPool
        ).mulDiv(price, 10 ** PRICE_DECIMALS, MathLib.Rounding.Down);

        currencyAmount = _fromPriceDecimals(currencyAmountInPriceDecimals, currencyDecimals, liquidityPool);
    }

As it's demonstrated above, MathLib.Rounding.Down, instead of MathLib.Rounding.Up is being used. Thus they are rounding down, instead of up.

Tools Used

Manual code review

Recommended Mitigation Steps

previewMint and previewWithdraw should round up.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/PoolManager.sol#L1-L351

Vulnerability details

Impact

According to Out of scope section: Rebase and fee-on-transfer tokens are not supported.

This message had been clarified on the Discord channel by the sponsor:

 — 
09.09.2023 10:02

ERC20 tokens that have rebasing logic (which change supply based on price differences) and fee-on-transfer logic
(which the actual transferred balance is slightly less because of a fee) will never be added through addCurrency,
and are thus not supported. If they would be added, some of the logic in the investment mgr wouldn't work correctly as it expects the balance to be static.

However, the documentation does not mention anything about:

• Upgradable tokens Tokens (e.g. USDC, USDT) which allows the token owner to make modification to the logic of the token. E.g. upgradable token may be upgraded and starts using fee-on-transfer mechanism

• Tokens with Blocklist Tokens (e.g. USDC, USDT) implements a blocklist. Admin can block the address (e.g. the Escrow address) which will make any transfer from and to forbidden. Thus tokens would stuck in the Escrow.

Proof of Concept

Documentation does not state anything about Upgradable tokens or Tokens with Blocklist - which means, that they can be added via addCurrency, and then - pools with that currency can be deployed. Whenever Upgradable token or Token with Blocklist will start behave abnormal (its behavior may lead to contract financial loss) - to mitigate the risk of deploying more pools with that token - currency should be removed. The PoolManager.sol, however, does not implement any function which allows to remove previously added currency.

Tools Used

Manual code review

Recommended Mitigation Steps

Implement removeCurrency - so when Upgradable token or Token with Blocklist will start behave abnormal (its behavior may lead to contract financial loss) - users at least, won't be able to deploy new pools for that token.

Assessed type

Other

During the security review phase, my main focus had been directed at the protocol's access control. Below sections of the analysis mostly describe issues related to its implementation.

The documentation provided by the Sponsors very comprehensively describes the authorization mechanism in the Access control paragraph. I do admire the fact, that so much documentation space has been dedicated to describe the Access Control mechanism, thus I had decided that my main focus during the security review - will be directed at evaluating every pros and cons of that mechanism and identify any potential issue related to it.

Approach taken

The first step of the security review was getting familiar with provided documentation. Fortunately, provided docs describes the topic of Access Control very extensively. However, the only inconsistency, I found during the documentation review process, is the below sentence on the contest's page: Authentication uses the ward pattern, in which addresses can be relied or denied to get access. Actually, in the context of the ward pattern – we are dealing with authorization, not the authentication, since wards are not authenticated, but just authorized to execute rely() an deny() methods.

I personally enjoyed the fact, that documentation describes multiple of emergency scenarios (e.g. someone triggers malicious scheduleUpgrade or pause() function). Having those emergency scenarios described and developing the protocol by keeping in mind that those emergency scenarios may occur – implies, how seriously the security of the protocol is treated.

After getting familiar with the documentation, I started to evaluate a security of the proposed mechanism, keeping my focus mostly on the files which implement or use the access control mechanism.

I evaluated the risks and threats (described in the Access Control Analysis Conclusion paragraph) related to Access Control mechanism. I also verified if described emergency scenarios are indeed working as described in the docs.

Another part of the code-base I focused on was the Restriction Manager (RestrictionManager.sol, Tranche.sol). Since documentation describes it as ERC1404 based contract that checks transfer restrictions - I got familiar with EIP-1404 and I verified if it is compliant with this standard. Identified risks and threats are described in Restriction Manager Analysis Conclusion.

Access Control Analysis Conclusion

The protocol solution for most of the described emergency scenarios lies in the time lock implementation. Nobody should be able to become a ward on any contract before the time lock. Based on that assumption – it is extremely crucial to make sure that the time lock mechanism is implemented properly. For many investors, the protocol's security and risk-mitigation mechanism are the main factor which allows them to decide if they want to invest their funds in the protocol. Having properly implemented time lock is indeed a relevant factor. However, the time lock mechanism can be fully disabled in Root.sol. While the time lock max length is defined to 4 weeks, it turns out that there is no lower limit when it comes to its duration. It can be set to 0 (or 1 second), which is basically equivalent to turning it off. Taking into consideration how important for security the time lock is – it should not be possible to disable it under any circumstances.

Another important issue is related to the ward pattern. Every contract should have at least one ward. Without any ward contract won't be functioning properly – since most of the methods are callable only by ward. None of the contracts enforce the rule of having at least one ward. Any ward can accidentally call deny() method on their own address (or accidentally pass their own address to deny()) and remove themself from being a ward. When the last ward removes themself from being a ward - protocol would become ward-less and broken. There should be additional mechanism implemented in the deny() method which guarantees, that ward cannot remove themself (e.g. passing msg.sender to that method should revert).

Every emergency scenarios described in the docs protect from adding malicious ward. There is, however, no emergency scenario when already added ward becomes malicious. It's crucial to notice, that malicious ward can actually do everything within the contract. This is, however, unavoidable. According to that – some centralization risk occurs. Adding a single EOA as ward is a centralization risk and a single point of failure. A single private key may be taken in a hack, or the sole holder of the key may become unable to retrieve the key when necessary. Moreover, the more wards are added, the higher chances are that one of them might become malicious. This risk should be evaluated every time the new ward is being added.

Restriction Manager Analysis Conclusion

As EIP-1404 standard requires, function detectTransferRestriction must be evaluated inside token's transfer and transferFrom. The logic of this function is left to the issuer. The developers decided to use this function to verify if the address is on the memberlist when receiving the token. The members are being added (or removed) via updateMember() function. I think that the better approach would be creating two additional functions: addMember() and removeMember(), instead of using updateMember() to both adding and/or removing members. The current implementation does not allow to immediately remove the member. They can only be removed in some point in the future:

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

If updateMember() (called with validUntil close to block.timestamp) stuck in the mem-pool and would be mined when validUntil > block.timestamp - updateMember would revert and user wouldn't be removed from the memberlist. Having a function which immediately removes the member from the memberlist is more secure approach.

Code base quality

As previously mentioned – this analysis mainly focuses on access control and Restriction Manager – thus the code-base quality is also evaluated within that context.

Most of the functions do not implement any NatSpec. Comments are also confusing. For example, the NatSpec of DelayedAdmin.sol is copied-pasted into PauseAdmin.sol (PauseAdmin contract is described as @title Delayed Admin), which obviously is not true.

The RestrictionManager.sol, even described as ERC1404 based contract - does not provide any explanation – why exactly it is implemented as ERC1404. By diving deeper into the code-base, this becomes clear – however, the code-base should include some comments, which will help to understand the overall purpose of the code, before actually reading it.

Time spent

12h (including 8h for reviewing the access control and Restriction Manager mechanisms).

Time spent:

12 hours