Centrifuge - imtybik's results

Lines of code

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

Vulnerability details

Impact

Incorrect calculations in _fromPriceDecimals(...) in the InvestmentManager contract may cause problems with other functions, because the return value is 0 due to the specifics of calculations in solidity.

Proof of Concept

uint8 public constant PRICE_DECIMALS = 18; value = _toUint128(_value / 10 ** (PRICE_DECIMALS - decimals));

If _value = 10**9 and decimals = 8 then value = 10**9 / 10 ** (18 - 8) = 10**9 / 10 ** 10 = 0

We can try with other values as well: _value = 10**11, decimals = 6 value = 10**11 / 10 ** (18 - 6) = 10**11 / 10 ** 12 = 0 This happens because we divide a smaller number by a larger one

I wrote an automated test using Foundry for all values of decimals <= 18 and _value <= 10**18. 3 values will be output to the log VALUE - DECIMALS - CALCULATED_VALUE

create file /test/POC_fromPriceDecimals.t.sol paste code:

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity 0.8.21;

import "forge-std/Test.sol";
import "forge-std/console2.sol";


contract POC_fromPriceDecimals is Test {

    uint8 public constant PRICE_DECIMALS = 18;

    function _toUint128(uint256 _value) internal pure returns (uint128 value) {
        if (_value > type(uint128).max) {
            revert("InvestmentManager/uint128-overflow");
        } else {
            value = uint128(_value);
        }
    }

    function _fromPriceDecimals(uint256 _value, uint8 decimals) internal view returns (uint128 value) {
        if (PRICE_DECIMALS == decimals) return _toUint128(_value);
        value = _toUint128(_value / 10 ** (PRICE_DECIMALS - decimals));
    }

    function test_fromPriceDecimals() public view {
        uint8 max_decimals = 18;
        uint8 max_value_n = 18;

        // decimals
        for (uint8 _decimals = 0; _decimals <= max_decimals; _decimals++) {

            // value
            for (uint8 value_n = 0; value_n <= max_value_n; value_n++) {
                
                uint256 value = 10 ** value_n;
                uint128 calculated_value = _fromPriceDecimals(value, _decimals);
                
                // log format: 
                // VALUE - DECIMALS - CALCULATED_VALUE
                console2.log(_decimals, value, calculated_value);
            }
        }
    }
}

run test:

$ forge test --mt test_fromPriceDecimals -vvv

Tools Used

Manual analysis, Foudry, Chisel

Recommended Mitigation Steps

Check the return value to be greater than 0, or make sure that the numerator is greater than the denominator using require()

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The interface used in src\InvestmentManager.sol

interface PoolManagerLike {
    ...
    function isAllowedAsPoolCurrency(uint64 poolId, address currencyAddress) external view returns (bool);
}

function returns bool, but the check is not executed. Token (currency) that is not supported in Centrifuge pool will be used in InvestmentManager::requestRedeem() and InvestmentManager::requestDeposit(). In other contracts, the check is performed. In this case, it was probably forgotten to be performed.

Line 124: poolManager.isAllowedAsPoolCurrency(lPool.poolId(), currency); Line 154: poolManager.isAllowedAsPoolCurrency(lPool.poolId(), lPool.asset());

Proof of Concept

At the moment PoolManager::isAllowedAsPoolCurrency() will always return true or revert() will occur. But if the PoolManager logic is changed so that true/false is returned and the address poolManager is updated with InvestmentManager::file(), the Token (currency) that is not supported in Centrifuge pool will be used in InvestmentManager::requestRedeem() and InvestmentManager::requestDeposit().

Tools Used

Manual analysis

Recommended Mitigation Steps

Change from Line 124: poolManager.isAllowedAsPoolCurrency(lPool.poolId(), currency); Line 154: poolManager.isAllowedAsPoolCurrency(lPool.poolId(), lPool.asset()); to Line 124: requre(poolManager.isAllowedAsPoolCurrency(lPool.poolId(), currency), "PoolManager/currency-not-supported") Line 154: requre(poolManager.isAllowedAsPoolCurrency(lPool.poolId(), lPool.asset()), "PoolManager/currency-not-supported")

Assessed type

Invalid Validation