possibility of reentrancy attack when `poolManger.sol#Transfer` called with malicious recipient contract address

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/PoolManager.sol#L128-L134 https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/PoolManager.sol#L257-L263

Vulnerability details

Impact

when users calls the transfer function in the poolManger.sol the transaction data will be send to the centrifuge chain first and then it will be back to the router and then direct it to thehandleTransfer function in poolManger.sol, user can make a reentrancy attack and drain the escrow contract. more details in the POC.

Proof of Concept

the function transfer is as follow:

  function transfer(
        //@audit possibility of reentrancy attack when the call comes back to the recipient
        address currencyAddress,
        bytes32 recipient,
        uint128 amount
    ) public {
        uint128 currency = currencyAddressToId[currencyAddress];
        require(currency != 0, "PoolManager/unknown-currency");
        //we give the amount to escrow till the transaction returns back from the centrifuge chain
        SafeTransferLib.safeTransferFrom(
            currencyAddress,
            msg.sender,
            address(escrow),
            amount
        );
        gateway.transfer(currency, msg.sender, recipient, amount);
    }

the call to this function will lead to call the gateway transfer function to encode data and send it to the centrifuge chain, however if we imagine that the transfer will happen using a currency that have transfer callback function on specific chain and if we put the recipient address to a contract which calls the currency callback function with logic to make a re call to the escrow to send all the tokens to our contract then we can drain the escrow contract, tokens with this functionality can be exist as we have xDai chain too which the USDC, WBTC, and WETH contained post-transfer callback procedures. even if the centrifuge did not support the xDai chain there is many other chains that the stable coin contracts are upgradable and can contain post-transfer callback function some day. to read more about the xDai problem check this link

however, when the call comes the handleTransfer will be called which the reentrancy attck can happen by our contract(recipient address)

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

        EscrowLike(escrow).approve(currencyAddress, address(this), amount);
        SafeTransferLib.safeTransferFrom(
            currencyAddress,
            address(escrow),
            recipient,
            amount
        );
    }

Tools Used

manual review

Recommended Mitigation Steps

add nonReentrant check to the public transfer function to prevent RA from happen in future.

Assessed type

Reentrancy

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #159

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid