Unchecked Return Values of PoolManager::isAllowedAsPoolCurrency()

[c4-submissions]

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

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #68

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[gzeon-c4]

isAllowedAsPoolCurrency revert if false, but as QA the bool should be checked

[c4-sponsor]

hieronx (sponsor) confirmed

[c4-judge]

gzeon-c4 marked the issue as grade-b