Comment and implementation mismatch in UserEscrow::transferOut

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The current implementation of UserEscrow::transferOut makes it impossible for anyone to call such a function if and only if he is not a ward (given the auth modifier):

    function transferOut(address token, address destination, address receiver, uint256 amount) external auth {
        ...

However, there is a @dev comment which says transferOut can only be initiated by the destination address or an authorized admin, but due to the modifier this won't be possible, as the only one who will be able to call this function will be a ward.

Proof of Concept

Pretty visual.

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

    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);
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the auth modifier for this contract and write at the top of the function something like:

-   function transferOut(address token, address destination, address receiver, uint256 amount) external auth {
+   function transferOut(address token, address destination, address receiver, uint256 amount) external {
+       require(wards[msg.sender] == 1 || msg.sender == destination, "UserEscrow/not-authorized");
        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);
    }

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as duplicate of #217

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as duplicate of #653

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

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