The implementation of UserEscrow.transferOut may cause the token of destination to be stolen by the receiver

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

User withdraws assets via LiquidityPool.withdraw/redeem, which can specify a receiver to receive assets and ultimately send assets to the receiver via userEscrow.transferOut. When the receiver and destination(caller) are different, transferOut will forcefully check whether the destination approves the maximum to the receiver. If not, then tx will be revert. This means that user needs to call asset.approve(receiver, type(uint256).max) in advance. In some cases, the receiver may not be the user's own wallet address. In this way, the receiver can transfer all the asset held by user's wallet, causing the user to suffer funds loss.

Proof of Concept

Let's take a look at the flow of withdraw, which is similar to redeem.

LiquidityPool.withdraw	//receiver != owner, owner=caller
  investmentManager.processWithdraw
    calculateRedeemPrice
    _calculateTrancheTokenAmount
    _redeem
      _decreaseRedemptionLimits
->    userEscrow.transferOut
        //receiver != destination(caller)
->      (ERC20Like(token).allowance(destination, receiver) == type(uint256).max)  //if this check is not met, tx will revert.

File: src\UserEscrow.sol
36:     function transferOut(address token, address destination, address receiver, uint256 amount) external auth {
37:         require(destinations[token][destination] >= amount, "UserEscrow/transfer-failed");
38:         require(
39:             /// @dev transferOut can only be initiated by the destination address or an authorized admin.
40:             ///      The check is just an additional protection to secure destination funds in case of compromized auth.
41:             ///      Since userEscrow is not able to decrease the allowance for the receiver,
42:             ///      a transfer is only possible in case receiver has received the full allowance from destination address.
43:->           receiver == destination || (ERC20Like(token).allowance(destination, receiver) == type(uint256).max),
44:             "UserEscrow/receiver-has-no-allowance"
45:         );
46:         destinations[token][destination] -= amount;
47: 
48:         SafeTransferLib.safeTransfer(token, receiver, amount);
49:         emit TransferOut(token, receiver, amount);
50:     }

L43, comments from dev: The check is just an additional protection to secure destination funds in case of compromized auth. This assumes that receiver is owned by the caller, however, this assumption does not apply to all cases. Consider the following scenario, assuming the asset is USDC:

1. One team has two members (bob and alice) and uses address A to deposit 10000e6 USDC into LiquidityPool.
2. As time goes by, they want to withdraw their assets. bob suggests the following steps:

• A calls USDC.approve(bob, type(uint256).max)
• A calls withdraw(5000e6, bob, A)
• A calls withdraw(5000e6, A, A)
• bob calls USDC.transferFrom(A, bob, allBalance), and bob gets all USDC.
• A calls USC.transfer(alice, allBalance), then tx revert. alice got nothing.

Alice is a non-technical user. So forcing the approve maximum caused Alice to suffer losses.

Tools Used

Manual Review

Recommended Mitigation Steps

Please do not use this method to force users to approve the maximum value to receiver.

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #217

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[securitygrid]

The intent of this contract is to ensure that the InvestmentManager cannot send funds to any other destination, except if that destination has explicitly set an allowance to send tokens elsewhere

In some cases, the receiver may not be the user's own wallet address. The current check is to force the user to approve the maximum value to the receiver. This may open the opportunity for the receiver to steal the user's token.
Please review the scenario described in this report. I'd like to hear opinions from @gzeon-c4 and @hieronx on such case.

[gzeon-c4]

The intent of this contract is to ensure that the InvestmentManager cannot send funds to any other destination, except if that destination has explicitly set an allowance to send tokens elsewhere

In some cases, the receiver may not be the user's own wallet address. The current check is to force the user to approve the maximum value to the receiver. This may open the opportunity for the receiver to steal the user's token. Please review the scenario described in this report. I'd like to hear opinions from @gzeon-c4 and @hieronx on such case.

This seems like a design decision, the user can also choose not to send it to someone they don't trust.