Centrifuge - Ch_301's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/RestrictionManager.sol#L27-L34

Vulnerability details

Description

As we know from README.md

Removing an investor from the memberlist in the Restriction Manager locks their tokens. This is expected behavior.

to check this, the modifier restricted is implemented in mint(),transferFrom() and transfer()

However, the restricted modifier in Tranche.sol will sub-call to restrictionManager.detectTransferRestriction()

File: RestrictionManager.sol

27:     // --- ERC1404 implementation ---
28:     function detectTransferRestriction(address from, address to, uint256 value) public view returns (uint8) {
29:         if (!hasMember(to)) {
30:             return DESTINATION_NOT_A_MEMBER_RESTRICTION_CODE;
31:         }
32: 
33:         return SUCCESS_CODE;
34:     }

You can notice it only checks the to address if has a member or not

So in case the from is removed investor he still able to transfer his Tranche Token to an active address (investor) in order to withdraw it later

Impact

The removed investors are still able to withdraw/transfer their locked Tranche Token

Tools Used

Manual Review

Recommended Mitigation Steps

check the from address if he is a member or not

Assessed type

Invalid Validation

Summary

Low Risk Issues

01- The Invest could get blacklisted in the supported currency 02- The UserEscrow.transferOut() will revert 03- Liquidity Pool is not ERC-4626 compatible contract 04- The tokens could have a different decimal in each chains 05- User could lose his Tranch tokens 06- The deployer could set delay to a value bigger than MAX_DELAY (4 weeks) 07- The admin could restart the scheduled delay period 08- No need for full allowance in case transferOut() get triggered by LiquidityPool.withdraw() with a different receiver

Low Risk Issues

01- The Invest could get blacklisted in the supported currency

in case _currency == USDT and user is blacklisted the transactions from Centrifuge chains will keep ever. and AXELAR will keep charging this protocol fee for every transaction

File: InvestmentManager.sol
SafeTransferLib.safeTransferFrom(_currency, address(escrow), user, currencyPayout);

GitHub: [L277-L292] (https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L277-L292)

02- The UserEscrow.transferOut() will revert

In case lpValues.maxWithdraw < _currency is true and there are not enough funds in UserEscrow.sol, the UserEscrow.transferOut() will revert

File: InvestmentManager.sol

        if (lpValues.maxWithdraw < _currency) {
            lpValues.maxWithdraw = 0;
        } else {
            lpValues.maxWithdraw = lpValues.maxWithdraw - _currency;
        }
        if (lpValues.maxRedeem < trancheTokens) {
            lpValues.maxRedeem = 0;
        } else {

GitHub: L639-L646

03- Liquidity Pool is not ERC-4626 compatible contract

from README.md

Liquidity Pool: A ERC-4626 compatible contract that enables investors to deposit and withdraw stablecoins to invest in tranches of pools.

However, the totalSupply() will return the supply of all the Liquidity Pools that are with the same Tranch Token. It should return only the supply of the current Liquidity Pools

File: LiquidityPool.sol
    function totalSupply() public view returns (uint256) {
        return share.totalSupply();
    }

GitHub: L283-L285

04- The tokens could have a different decimal in each chain.

Because some tokens have a different decimal in a different chain So, in case currencyAddress has a different decimal in the Centrifuge Chain. the same thing with handleTransfer() it just takes amount and transfers it to the recipient

File: PoolManager.sol

    function transfer(address currencyAddress, bytes32 recipient, uint128 amount) public {
        uint128 currency = currencyAddressToId[currencyAddress];
        require(currency != 0, "PoolManager/unknown-currency");

        SafeTransferLib.safeTransferFrom(currencyAddress, msg.sender, address(escrow), amount);
        gateway.transfer(currency, msg.sender, recipient, amount);
    }

GitHub: L128-L134

05- User could lose his Tranch tokens

In case the user passes a non-supported destinationChainId in transferTrancheTokensToEVM(). he will lose his funds

File: PoolManager.sol

    function transferTrancheTokensToEVM(
        uint64 poolId,
        bytes16 trancheId,
        uint64 destinationChainId,
        address destinationAddress,
        uint128 amount
    ) public {
        TrancheTokenLike trancheToken = TrancheTokenLike(getTrancheToken(poolId, trancheId));
        require(address(trancheToken) != address(0), "PoolManager/unknown-token");

        trancheToken.burn(msg.sender, amount);
        gateway.transferTrancheTokensToEVM(
            poolId, trancheId, msg.sender, destinationChainId, destinationAddress, amount
        );
    }

GitHub: L149-L163

06- The deployer could set delay to a value bigger than MAX_DELAY (4 weeks)

The deployer could set delay to a value bigger than MAX_DELAY (4 weeks)

File: Root.sol

    constructor(address _escrow, uint256 _delay) {
        escrow = _escrow;
        delay = _delay;

GitHub: L34-L36

07- The admin could restart the scheduled delay period

admin could invoke scheduleRely() function two times for the same target. this will restart the scheduled delay period that was passed

File: Root.sol

    function scheduleRely(address target) external auth {
        schedule[target] = block.timestamp + delay;
        emit RelyScheduled(target, schedule[target]);
    }

GitHub: L65-L68

08- No need for full allowance in case transferOut() get triggered by LiquidityPool.withdraw() with a different receiver

users don't need to set any allowance or full allowance to invoke LiquidityPool.withdraw() with a different receiver

File: UserEscrow.sol

            receiver == destination || (ERC20Like(token).allowance(destination, receiver) == type(uint256).max),
            "UserEscrow/receiver-has-no-allowance"

GitHub: L43-L44