User's tokens can get locked in UserEscrow.sol for an unknown duration of time... potentially forever.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Users token in some instances could be locked off in UserEscrow.sol with no way to rescue these tokens as this is all in the hands of the third party

Proof of Concept

From docs we know that, for tokens that have been requestRedeem()ed, but not yet withdrawn, these are held in the UserEscrow contract. The key design principle of this contract is that once tokens are transferred in, they are locked to a specific destination, and can only be transferred out to this destination. Even a ward on the UserEscrow contract cannot transfer tokens to any other destination.

As seen above, the UserEscrow.sol has an interesting implementation, only the destination can initiate transferOuts even wards can't side step this do note that massive adoption is made for integrating stable coins to this protocol, meaning that the chances of the token that's locked in UserEscrow.sol to be USDC/USDT is very high with both tokens combined presently having ~$110 billion market cap.

Now both USDC/USDT have a blocklisting mechanism where they can add any address to this list and as such stop interactions with their tokens from this address, using USDC as a case study, its got a notBlacklisted() modifier that's required for functions such as transfer(), transferFrom() and approve.

Take a look at 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 seen, where as the end receiver could be a different address than the destination, in a case where destination is already blacklisted by Circle, they can't make any approvals of their tokens to other accounts as USDC's approve() also requires both the spender and the owner to not be blacklisted

Tool used

Manual Review

Recommended Mitigation Steps

For tokens that implement blacklisting holders, I advise allowing destination to specify a fresh address to receive the tokens locked in UserEscrow.sol i.e there shouldn't be a need to approve said address first.

Assessed type

Token-Transfer

[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)