Centrifuge - BARW's results

Lines of code

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

Vulnerability details

Impact

Even if a user is no longer on the member list he can still send his tranch tokens to an address that is on the member list and can redeem them through the other address

Proof of Concept

The member list for a tranch token is a whitelist of addresses that determines which addresses can handle tranch tokens. It is also used to prevent users to redeem tranch tokens by checking if the sender is on the member list when sending a redeem request. The goal here is to freeze the users tranch tokens in case of a malicious activity. The problem arises in the check that is done for a transaction when sending tranch tokens from one account to another. The restricted modifier on Tranch.transferFrom() calls detectTransferRestriction(from, to, value):

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;
    }

This function only check if the to address is on the member list, not if the from address is there. This enables an address that is not a member to send its tranch tokens to an member of the list and thereby bypass the restrictions for redemption of tranch tokens.

Example:

Alice steals a huge amount of tranch tokens from different users. This is detected and centrifuge removes the address of Alice from the member list to prevent her from redeeming the stolen tranch tokens. Bob is still on the member list. Shortly before the epoch of the RWA pool on centrifuge ends Alice transfers her stolen tranch tokens to Bob. He sends a redeem request to centrifuge that goes through since he is on the member list. The request is executed shortly after when the epoch of the RWA pool ends, and Bob can redeem the stable coins for the tranch tokens that should have been locked in Alice’s account.

Tools Used

Manual review

Recommended Mitigation Steps

Add a check to detectTransferRestriction that makes sure that the from address is also a member of the list. If any of the from or to addresses is not on the list, the function should return false and a transfer should not be possible.

Assessed type

Token-Transfer

QA-01: Missing check in the constructor of Root.sol if the initially set delay is smaller than the MAX_DELAY

Link to code: https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/Root.sol#L34-L40

Description:

There is no check in the constructor of Root.sol that makes sure that the initially set delay is smaller than the MAX_DELAY. This can lead to delays in the launch of the project in case the dalay is set to more than the MAX_DELAY since only wards of root can change this value and it would take more than MAX_DELAY to set a new ward for the root contract. Consider adding a check that ensures that the initially set delay is smaller than MAX_DELAY

QA-02: redundant check at PoolManager.sol deployLiquidityPool.

If the pool doesn't exist, the function will revert at its beginning. Here's the relevant code:

    function deployLiquidityPool(uint64 poolId, bytes16 trancheId, address currency) public returns (address) {
        // @info check if the tranch exist
        Tranche storage tranche = pools[poolId].tranches[trancheId];
        require(tranche.token != address(0), "PoolManager/tranche-does-not-exist"); // Tranche must have been added

        // @info check is currency is added
        require(isAllowedAsPoolCurrency(poolId, currency), "PoolManager/currency-not-supported"); // Currency must be supported by pool

        // @info check if liquidityPool is deployed, so this function can't be called twice with same parameters
        address liquidityPool = tranche.liquidityPools[currency];
        require(liquidityPool == address(0), "PoolManager/liquidityPool-already-deployed");
        // @audit redundant check, this line will never be executed
        require(pools[poolId].createdAt != 0, "PoolManager/pool-does-not-exist");

QA-03: The functions _stringToBytes32 and _stringToBytes128 are missing length checks.

Unexpected results may occur if the string length is greater than or equal to 32 or 128, respectively.

POC

    function testMsgForwarding() public {
        string memory test = "thisisatet";
        bytes32 result1 = _stringToBytes32(test);
        emit log_bytes32(result1);
        string memory string1 = _bytes32ToString(result1);
        console.log("test: %s string1 : %s ", string1);

        string memory stringWithLeng40 = "1234567890123456789012345678901234567890";

        bytes32 result2 = _stringToBytes32(stringWithLeng40);
        string memory string2 = _bytes32ToString(result2);
        console.log("stringWithLeng40 : %s result : %s", stringWithLeng40, string2);

        emit log_bytes32(result2);
    }