code-423n4 / 2023-09-centrifuge-findings

16 stars 14 forks source link

There is no concept of a requestDeposit, requestRedeem receipt made on the source chain, resulting in no recovery proces escrowed funds in the event of bridge or Centrifuge fall. #794

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L214-L217 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/LiquidityPool.sol#L231-L234 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/InvestmentManager.sol#L148-L172 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/gateway/Gateway.sol#L224-L236

Vulnerability details

Impact

There is no kind of request receipt on the EVM chain at all. The InvenstmentManager#requestDeposit simply puts users tokens in the escrow and then the request gets routed to the Gateway and later to the outgoingRouter. The flow goes as follows

1.Buyer calls LiquidityPool#requestDeposit with the currency amount on an appropriate

  1. Which calls InvenstmentManager#requestDepositwhich
    1. Validates the LiqudityPoolz and it’scurrency`
    2. Validates if buyer is on the allowlist
    3. Transfers the currency amount from buyer to the escrow
    4. Calls gateway#increaseInvestOrder
  2. Gateway calls outgoingRouter#send 



    function requestDeposit(uint256 assets, address owner) public withApproval(owner) {
        investmentManager.requestDeposit(assets, owner);
        emit DepositRequested(owner, assets);
    }
    
    function requestDeposit(uint256 currencyAmount, address user) public auth {
        address liquidityPool = msg.sender;
        LiquidityPoolLike lPool = LiquidityPoolLike(liquidityPool);
        address currency = lPool.asset();
        uint128 _currencyAmount = _toUint128(currencyAmount);
    
        // Check if liquidity pool currency is supported by the Centrifuge pool
        poolManager.isAllowedAsPoolCurrency(lPool.poolId(), currency);
        // Check if user is allowed to hold the restricted tranche tokens
        _isAllowedToInvest(lPool.poolId(), lPool.trancheId(), currency, user);
        if (_currencyAmount == 0) {
            // Case: outstanding investment orders only needed to be cancelled
            gateway.cancelInvestOrder(
                lPool.poolId(), lPool.trancheId(), user, poolManager.currencyAddressToId(lPool.asset())
            );
            return;
        }
    
        // Transfer the currency amount from user to escrow. (lock currency in escrow).
        SafeTransferLib.safeTransferFrom(currency, user, address(escrow), _currencyAmount);
    
        gateway.increaseInvestOrder(
            lPool.poolId(), lPool.trancheId(), user, poolManager.currencyAddressToId(lPool.asset()), _currencyAmount
        );
    }
    
    function increaseInvestOrder(
        uint64 poolId,
        bytes16 trancheId,
        address investor,
        uint128 currency,
        uint128 currencyAmount
    ) public onlyInvestmentManager pauseable {
        outgoingRouter.send(
            Messages.formatIncreaseInvestOrder(poolId, trancheId, _addressToBytes32(investor), currency, currencyAmount)
        );
    }

The request gets recorded only when Centrifuge calls back with handleExecutedCollectInvest and handleExecutedCollectRedeem

Concept of recording requests does not exists anywhere in the EVM code. As a result full trust is put into Centrifuge and the bridging infrastructure. Bridges are in no way as secure as a blockchain and as a 3rd party infra not in Centrifuge’s control;

In case any of them fails there is:

  1. no onchain process to rectify the issue
  2. No onchain record to facilitate any manual solutions

Parsing events might help solving such a situation, it would be tedious and not reliable though

Tools Used

Manual review

Recommended Mitigation Steps

Implement the concept of request accounting. It can be done in different ways. A lightweight in line with the style of the codebase would be to store users escrowed funds amount in the ‘Escrowor InvestmentManager` and update it on requests, deposits/mints redeems/withdraws.

Assessed type

Other

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #26

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid