handleExecutedDecreaseInvestOrder may revert in some cases

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L291

Vulnerability details

Impact

Users can request decreasing the outstanding deposit orders via decreaseDepositRequest which will send a cross-chain message to the Centrifuge chain. After the message is processed, Centrifuge's bot will decrease the outstanding deposit orders and send another cross-chain message back to the source chain to notify that the request has been processed. This will eventually call handleExecutedDecreaseInvestOrder, which internally transfers the asset from escrow to user via asset.transferFrom. Assets can be tokens with blacklist mechanisms such as USDC and USDT. If the user is blacklisted, it will cause USDC.transferFrom revert. Therefore, the message cannot be successfully processed, which means that the user cannot obtain the decreased asset, resulting in a loss of funds.

Proof of Concept

When a message of type Call.ExecutedDecreaseInvestOrder is executed by relayer, the whole flow is as follows:

Router.execute	//src\gateway\routers\axelar\Router.sol
  gateway.handle
    investmentManager.handleExecutedDecreaseInvestOrder
      ......some checks skipped
      SafeTransferLib.safeTransferFrom(_currency, address(escrow), user, currencyPayout)

Let's take a look at the code of handleExecutedDecreaseInvestOrder:

File: src\InvestmentManager.sol
277:     function handleExecutedDecreaseInvestOrder(
278:         uint64 poolId,
279:         bytes16 trancheId,
280:         address user,
281:         uint128 currency,
282:         uint128 currencyPayout
283:     ) public onlyGateway {
284:         require(currencyPayout != 0, "InvestmentManager/zero-payout");
285: 
286:         address _currency = poolManager.currencyIdToAddress(currency);
287:         address liquidityPool = poolManager.getLiquidityPool(poolId, trancheId, _currency);
288:         require(liquidityPool != address(0), "InvestmentManager/tranche-does-not-exist");
289:         require(_currency == LiquidityPoolLike(liquidityPool).asset(), "InvestmentManager/not-tranche-currency");
290: 
291:->       SafeTransferLib.safeTransferFrom(_currency, address(escrow), user, currencyPayout);
292:     }

L291, if _currency is USDC or USDT and user is blacklisted, tx will revert.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended not to transfer the asset directly to the user, but to record the amount of the asset and provide the claim function to allow the user to specify the receiver.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #32

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-c

[c4-judge]

gzeon-c4 marked the issue as grade-b