Centrifuge - Aymen0909's results

Lines of code

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

Vulnerability details

Impact

The _DOMAIN_SEPARATOR is incorrectly calculated in ERC20.constructor as the _calculateDomainSeparator function uses the name variable which was not set yet, thus the found _DOMAIN_SEPARATOR will be wrong and as it's an immutable variable it cannot be changed in the future.

This issue will affect any signature verification functionality and will block the use of the permit function.

Proof of Concept

The issue occurs in the ERC20.constructor function below :

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

    deploymentChainId = block.chainid;
    // @audit name is not yet set (will be "") => _DOMAIN_SEPARATOR is wrong
    _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
}

As it can be seen from the code above, the _calculateDomainSeparator function is called to calculate before _DOMAIN_SEPARATOR which is an immutable variable, the _calculateDomainSeparator function is as follows :

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)), //@audit name not yet set in constructor
                keccak256(bytes(version)),
                chainId,
                address(this)
            )
        );
}

The function uses the name variable in the hash calculation but as we can see from the constructor the name value is not yet set and so it will be equal to "", this will cause the _DOMAIN_SEPARATOR to be incorrectly calculated and because its an immutable variable this error cannot be rectified.

_DOMAIN_SEPARATOR is used in the permit function for calculating the digest hash (when block.chainid == deploymentChainId) which is used to verify signatures, because _DOMAIN_SEPARATOR is wrong the permit function may not work as expect and could even be blocked.

Tools Used

Manual review

Recommended Mitigation Steps

To resolve this issue I recommend to set the name variable in the constructor before calling _calculateDomainSeparator :

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

    deploymentChainId = block.chainid;

    // @audit set name before _DOMAIN_SEPARATOR calculation
    name = tokenName;
    _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
}

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L427-L441 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L467-L480 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L591-L603 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L692

Vulnerability details

Impact

Malicious actors can exploit the accessibility of the LiquidityPool.deposit function to anyone and the potential for zero rounding in the _calculateTrancheTokenAmount function. They can utilize these vulnerabilities to manipulate the orderbook.maxDeposit value of users, potentially reducing it to zero. This manipulation can effectively deny certain users from receiving the correct amount of tranche tokens corresponding to their deposits.

Proof of Concept

The issue starts in the LiquidityPool.deposit function which has no access control and thus can be called by anyone, an attacker can provide any asset amount assets and recipient address receiver as argument to the function and is able to call investmentManager.processDeposit function :

function deposit(
    uint256 assets,
    address receiver
) public returns (uint256 shares) {
    shares = investmentManager.processDeposit(receiver, assets);
    emit Deposit(address(this), receiver, assets, shares);
}

The processDeposit function is given in the code below :

function processDeposit(
    address user,
    uint256 currencyAmount
) public auth returns (uint256 trancheTokenAmount) {
    address liquidityPool = msg.sender;
    uint128 _currencyAmount = _toUint128(currencyAmount);
    require(
        (_currencyAmount <= orderbook[user][liquidityPool].maxDeposit &&
            _currencyAmount != 0),
        "InvestmentManager/amount-exceeds-deposit-limits"
    );

    uint256 depositPrice = calculateDepositPrice(user, liquidityPool);
    require(depositPrice != 0, "LiquidityPool/deposit-token-price-0");

    uint128 _trancheTokenAmount = _calculateTrancheTokenAmount(
        _currencyAmount,
        liquidityPool,
        depositPrice
    );
    // @audit no check if _trancheTokenAmount != 0

    _deposit(_trancheTokenAmount, _currencyAmount, liquidityPool, user);
    trancheTokenAmount = uint256(_trancheTokenAmount);
}

The function calls the calculateDepositPrice to get the users's deposit price which depends on both values : orderbook.maxDeposit and orderbook.maxMint.

After that to get the amount of tranche token to be transferred to the user, _calculateTrancheTokenAmount is called :

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

As we can see the function makes a call to _toPriceDecimals in order to convert amount to correct decimals (18), the _toPriceDecimals is presented below :

function _fromPriceDecimals(
    uint256 _value,
    uint8 decimals,
    address liquidityPool
) internal view returns (uint128 value) {
    if (PRICE_DECIMALS == decimals) return _toUint128(_value);
    //@audit zero rounding risk
    value = _toUint128(_value / 10 ** (PRICE_DECIMALS - decimals));
}

The issue now is that the _fromPriceDecimals function can be subject to zero amount rounding, as you can see if we have : _value < 10 ** (PRICE_DECIMALS - decimals) then the returned value will equal to zero due to the rounding down, this issue can be exploited by an attacker who will always make sure that input currency amount to the LiquidityPool.deposit function will always result in the zero amount rounding.

The returned value from _fromPriceDecimals is stored in the return variable trancheTokenAmount in _calculateTrancheTokenAmount and this value is directly given to the internal _deposit function in processDeposit :

uint128 _trancheTokenAmount = _calculateTrancheTokenAmount(
    _currencyAmount,
    liquidityPool,
    depositPrice
);
// @audit no check if _trancheTokenAmount != 0

_deposit(_trancheTokenAmount, _currencyAmount, liquidityPool, user);

The _deposit function will not check if _trancheTokenAmount is different from zero and will directly proceed to the transfer and because transferFrom doesn't revert on zero transfer the call will not revert and the function will also decrease the values of maxDeposit and maxMint of the user :

function _deposit(
    uint128 trancheTokenAmount,
    uint128 currencyAmount,
    address liquidityPool,
    address user
) internal {
    LiquidityPoolLike lPool = LiquidityPoolLike(liquidityPool);

    //@audit will decrease the user's maxDeposit and maxMint
    _decreaseDepositLimits(
        user,
        liquidityPool,
        currencyAmount,
        trancheTokenAmount
    ); // decrease the possible deposit limits
    require(
        lPool.checkTransferRestriction(msg.sender, user, 0),
        "InvestmentManager/trancheTokens-not-a-member"
    );

    //@audit No check for trancheTokenAmount before transfer
    require(
        lPool.transferFrom(address(escrow), user, trancheTokenAmount),
        "InvestmentManager/trancheTokens-transfer-failed"
    );

    emit DepositProcessed(liquidityPool, user, currencyAmount);
}

So now that the overall call has ended the honest user (receiver) has not received any tranche token amount (received zero from the transfer call) and the attacker managed to decrease the user's maxDeposit value, the attacker can perform this attack many times (using a for loop with the help of a bot) to decrease the user's maxDeposit value even further till zero.

The result is that when the honest user tries to call the processDeposit function to get his tranche tokens, he will not be able give his full currency amount that he deposited as input because the first check in the processDeposit function will revert (check on _currencyAmount <= maxDeposit) as the attacker has already deceased maxDeposit, due to this the user in the end will get a very small amount of tranche token not the one he was expecting and this amount could even be 0 if the attacker decreased the user's maxDeposit to 0.

This attack is not complex to execute and it doesn't cost the attacker anything, the only thing that the attacker must do is to calculate each time (for every call) the value of _currencyAmount (input to processDeposit from deposit function) that will make the _fromPriceDecimals function and subsequently _calculateTrancheTokenAmount function return zero when called in processDeposit (due to the zero rounding mentioned above), and this is not complicated as the function _calculateTrancheTokenAmount relies on the deposit price in its calculation that depends on the values of maxDeposit and maxMint which are always known for all users.

NOTE : this attack will have higher impact for tranche tokens with a small decimals (tranche token decimals value can be set to any value (below 18) when the Tranche is added in PoolManager), as this will help the attacker decrease the user's maxDeposit in fewer calls because he will be able to give higher currency amount as input (because 10 ** (PRICE_DECIMALS - decimals) will be bigger allowing higher value amounts to trigger the zero rounding).

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this attack vector I recommend to check that the value returned from _calculateTrancheTokenAmount is always different from zero :

function processDeposit(
    address user,
    uint256 currencyAmount
) public auth returns (uint256 trancheTokenAmount) {
    address liquidityPool = msg.sender;
    uint128 _currencyAmount = _toUint128(currencyAmount);
    require(
        (_currencyAmount <= orderbook[user][liquidityPool].maxDeposit &&
            _currencyAmount != 0),
        "InvestmentManager/amount-exceeds-deposit-limits"
    );

    uint256 depositPrice = calculateDepositPrice(user, liquidityPool);
    require(depositPrice != 0, "LiquidityPool/deposit-token-price-0");

    uint128 _trancheTokenAmount = _calculateTrancheTokenAmount(
        _currencyAmount,
        liquidityPool,
        depositPrice
    );
    // @audit check that _trancheTokenAmount != 0
    require(_trancheTokenAmount != 0,"InvestmentManager/zero-tranche-amount");

    _deposit(_trancheTokenAmount, _currencyAmount, liquidityPool, user);
    trancheTokenAmount = uint256(_trancheTokenAmount);
}

Assessed type

Math