Centrifuge - josephdara's results

Lines of code

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

Vulnerability details

Impact

The domain separator is computed at deployment time. However at the point it is computed the values needed have not been set.

//In the constructor
        _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
..
..
 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)
            )
        );
    }

Proof of Concept

As seen above the function used to set the domain separator uses the name, version, and chainID, However the value of the name is not set in the constructor. They are set after deployment. Hence this causes the DOMAIN_SEPARATOR() function to return a different value from what should have been originally computed if the name was available.

The function DOMAIN_SEPARATOR() returns the wrong _DOMAIN_SEPARATOR every time it is called with the same chainID.

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

Tools Used

Manual Review

Recommended Mitigation Steps

Do not compute the Domain Separator in the constructor, compute it after the name has been set

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L125-L132 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L323-L347

Vulnerability details

Impact

Rounding of shares to assets do not conform to the ERC4626 standard: EIP 4626's Security Considerations (https://eips.ethereum.org/EIPS/eip-4626)

Finally, ERC-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.

However in the LiquidityPool and the InvestmentManager, the convertToShares and convertToAssets are both rounded down. The resulting effect of this is that users get less tokens when they are redeeming their shares.

Proof of Concept

  /// @dev Calculates the amount of shares / tranche tokens that any user would get for the amount of currency / assets provided.
    ///      The calculation is based on the tranche token price from the most recent epoch retrieved from Centrifuge.
    function convertToShares(uint256 _assets, address liquidityPool) public view auth returns (uint256 shares) {
        (uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);
        uint128 assets = _toUint128(_assets);

        shares = assets.mulDiv(
            10 ** (PRICE_DECIMALS + trancheTokenDecimals - currencyDecimals),
            LiquidityPoolLike(liquidityPool).latestPrice(),
            MathLib.Rounding.Down
        );
    }

    /// @dev Calculates the asset value for an amount of shares / tranche tokens provided.
    ///      The calculation is based on the tranche token price from the most recent epoch retrieved from Centrifuge.
    function convertToAssets(uint256 _shares, address liquidityPool) public view auth returns (uint256 assets) {
        (uint8 currencyDecimals, uint8 trancheTokenDecimals) = _getPoolDecimals(liquidityPool);
        uint128 shares = _toUint128(_shares);

        assets = shares.mulDiv(
            LiquidityPoolLike(liquidityPool).latestPrice(),
            10 ** (PRICE_DECIMALS + trancheTokenDecimals - currencyDecimals),
            MathLib.Rounding.Down //@audit-issue SH=hould round up
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend that the convertToAssets should round up

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Impact

The detectTransferRestriction takes in all the necessary parameters:

  function detectTransferRestriction(address from, address to, uint256 value) public view returns (uint8)

However it only performs a check on the address to The restriction used here is timebased, however a user can continuing using, swapping, transferring and even redeeming their tranche tokens long after their restriction time has passed.

Proof of Concept

This is caused by the function below

    function hasMember(address user) public view returns (bool) {
        if (members[user] >= block.timestamp) {
            return true;
        }
        return false;
    }

    function detectTransferRestriction(address from, address to, uint256 value) public view returns (uint8) {
        if (!hasMember(to)) {
            return DESTINATION_NOT_A_MEMBER_RESTRICTION_CODE;
        }

        return SUCCESS_CODE;
    }

This function detectTransferRestriction checks only the destination, not the origin address

Tools Used

Manual review

Recommended Mitigation Steps

Implement checks for the from address

Assessed type

ERC20