In order to send redeemed funds to other receipient owner have to provide full allowance to it

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

In order to send redeemed funds to other receipient owner have to provide full allowance to it

Proof of Concept

When user requests redeem, then assets are sent to the user escrow.

Then when user redeems, he can provide other recipient of assets.
In order to send assets to the recipient userEscrow.transferOut function is called.

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

    function transferOut(address token, address destination, address receiver, uint256 amount) external auth {
        require(destinations[token][destination] >= amount, "UserEscrow/transfer-failed");
        require(
            /// @dev transferOut can only be initiated by the destination address or an authorized admin.
            ///      The check is just an additional protection to secure destination funds in case of compromized auth.
            ///      Since userEscrow is not able to decrease the allowance for the receiver,
            ///      a transfer is only possible in case receiver has received the full allowance from destination address.
            receiver == destination || (ERC20Like(token).allowance(destination, receiver) == type(uint256).max),
            "UserEscrow/receiver-has-no-allowance"
        );
        destinations[token][destination] -= amount;


        SafeTransferLib.safeTransfer(token, receiver, amount);
        emit TransferOut(token, receiver, amount);
    }

As you can see in case if receiver != destination, then function checks that destination has provided max allowance to the receiver.

This is wrong for several reasons. User doesn't need to provide allowance as it's his choice to whom he send redeemed assets. Because it's only destination user can initiate redeem, then he shouldn't provide allowance.
Another concern, is that in case if i need to send assets to someone and i decided that it's more convinient for me to do through redeem, then i will provide max allowance to the recipient. At this moment i have a risk, that he will steal assets that i already have on balance.

Also as LiquidityPool is extending ERC4626, there is no such requirement for assets receiver.

Tools Used

VsCode

Recommended Mitigation Steps

I don't see the need of allowance check.

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

hieronx (sponsor) disputed

[hieronx]

This is incorrect. The destination is not triggering this function, rather it's the InvestmentManager (as the function is auth). 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 (this is the fail safe implemented here).

[gzeon-c4]

destination is the user, receiver is the actual receiving address
it make sense to have the user approving the receiving address to receive the proceed

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid