User funds can be stuck into escrow forever

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/Escrow.sol#L14-L27

Vulnerability details

Impact

The Escrow.sol smart contract receive assets from investors and send back those assets to investors when they withdraw. However some tokens (e.g. USDC, USDT) have a contract level admin controlled address blocklist. If an address is blocked, then transfers to and from that address are forbidden. see here
If a user invest in a pool using one of those tokens and after get added to blocklist by these tokens owner, the invested funds get trapped into escrow contract forever.

Proof of Concept

Here is an example of blockable token:
https://github.com/d-xo/weird-erc20/blob/04fbd6411f201da59a895dd6d3506913a525dfa9/src/BlockList.sol#L1-L29

// Copyright (C) 2020 d-xo
// SPDX-License-Identifier: AGPL-3.0-only


pragma solidity >=0.6.12;


import {ERC20} from "./ERC20.sol";


contract BlockableToken is ERC20 {
    // --- Access Control ---
    address owner;
    modifier auth() { require(msg.sender == owner, "unauthorised"); _; }


    // --- BlockList ---
    mapping(address => bool) blocked;
    function block(address usr) auth public { blocked[usr] = true; }
    function allow(address usr) auth public { blocked[usr] = false; }


    // --- Init ---
    constructor(uint _totalSupply) ERC20(_totalSupply) public {
        owner = msg.sender;
    }


    // --- Token ---
    function transferFrom(address src, address dst, uint wad) override public returns (bool) {
        require(!blocked[src], "blocked");
        require(!blocked[dst], "blocked");
        return super.transferFrom(src, dst, wad);
    }
}

As we can see if an address is blocked it can never receive or transfer tokens again. This situation result in user funds being trapped everywhere he invested ( in this case Cantrifuge ).
However Centrifuge did not implement a withdraw function in the Escrow.sol contract to allow an emergency withdraw of tokens that can potentially be stuck inside forever.

Tools Used

Manual review

Recommended Mitigation Steps

Add an emergency admin withdraw function to withdraw an user funds if he gets blocked by asset contract owner. Here is an example:

function withdraw(address asset, address user) external onlyAdmin {
    IERC20(asset).transfer(msg.sender, balances[user]);
}
modifier onlyAdmin() {
 if(msg.sender != admin) revert();
_;
}

For more transparancy this function should only be called by admin after a governance vote.

Assessed type

ERC20

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

hieronx marked the issue as disagree with severity

[hieronx]

This is a a general issue with blacklisted tokens, and many DeFi protocols will have similar behaviour (a protocol like MakerDAO also does not have such emergency withdraw functions). By adding such a function, centralization risk is significantly increased, which is not worth it. I would consider this low.

[c4-sponsor]

hieronx (sponsor) acknowledged

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-b

[0xbtk]

I believe that this don't falls under "Low risk" as there is a risk of permanent frozen funds:

Low risk (e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments).

Medium severity seems appropriate based on c4 severity categorization:

Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

wdyt @gzeon ?

[gzeon-c4]

I think an external blacklist is clearly out-of-scope.