Centrifuge - 0xTiwa's results

Lines of code

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

Vulnerability details

Impact

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 contract itself during calculations over its users: If it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or 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 it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up. Meanwhile in our InvestmentManager contract, both convertToShares() and convertToAssets() were both rounded down whereas it shouldn't be so, convertToAssets() should be rounded up.

Proof of Concept

In our convertToShares(), it's calculating the amount of shares or tranche tokens that any user would get for the amount of currency or assets provided, therefore, we are depositing not withdrawing and it should be rounded down.

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
        );
    }

But in the convertToAssets(), it's calculating the asset value for an amount of shares / tranche tokens provided. Since it's withdrawing not depositing it should be rounded up.

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),
            //@audit should round up because you are withdrawing not depositing
            MathLib.Rounding.Down
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Follow the due procedure recommended by the ERC4626 standard and use a rounding up(Rounding.up) for the convertToAssets()

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),
            //@audit should round up because you are withdrawing not depositing
            MathLib.Rounding.Up
        );
    }

Assessed type

ERC4626