Centrifuge - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L48

Vulnerability details

Impact

Name field is not included to the domain separator. Which is not correct according to EIP712. As result users may not trust to sign such message. And also users, who will craft signed messages themselves will not be able to validate them.

Proof of Concept

Each Tranche token extends ERC20 token. ERC20 token allows users to sign permits. DOMAIN_SEPARATOR is calculated on construction of ERC20. It includes name field, which is not set yet, so empty name is used. Name is set after construction.

Because of that crafted DOMAIN_SEPARATOR doesn't follow EIP-712. As result when user will sign messages, crafted by centrifuge app, they will see that domain separator doesn't contain name, that tranche token has. Because of that they will likely reject signing.

Another thing is that some people will craft signed messages themselves, which means that they will craft it according to the EIP712 and will include real Tranche token's name. As result, permit will not work in such case.

I have created test which shows that domain separator is created with empty name. In case if you sign permit with empty name, then it will work, in case if you will do that with correct name, then it will fail. Just put it into ERC20.t.sol.

function testPermitWrong() public {
        ERC20 testToken = new ERC20(18);
        //name is provided to the token after creation, when domain separator is already created
        testToken.file(bytes32("name"), "token");

        string memory name = "";
        console.log("we try with empty name is inside domain separator, it works");
        permitWithName(name, testToken);

        name = testToken.name();
        console.log("we try with token name as it will be done by clients, it fails");
        permitWithName(name, testToken);
    }

    function permitWithName(string memory name, ERC20 testToken) public {
        uint256 privateKey = 0xBEEF;
        address owner = vm.addr(privateKey);

        bytes32 domain = keccak256(
            abi.encode(
                keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
                keccak256(bytes(name)),
                keccak256(bytes(testToken.version())),
                block.chainid,
                address(testToken)
            ));
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(
            privateKey,
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    domain,
                    keccak256(abi.encode(PERMIT_TYPEHASH, owner, address(0xCAFE), 1e18, 0, block.timestamp))
                )
            )
        );

        vm.expectEmit(true, true, true, true);
        emit Approval(owner, address(0xCAFE), 1e18);
        testToken.permit(owner, address(0xCAFE), 1e18, block.timestamp, v, r, s);

        assertEq(testToken.allowance(owner, address(0xCAFE)), 1e18);
    }

Tools Used

VsCode

Recommended Mitigation Steps

Provide name of token into constructor.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L84 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/PoolManager.sol#L223

Vulnerability details

Impact

_DOMAIN_SEPARATOR is not updated, when name of token is changed

Proof of Concept

This is similar to the one that i have already submited. However, i believe that this is separate issue, because in case if first one is fixed, this one still remains. While first bug is about wrong initialization, this one is about updating metadata of contract which is a part of functionality of protocol. Here is similar issue on c4.

According to the EIP-712 in case if name, version or any other fields in the _DOMAIN_SEPARATOR is changed, then all previous signatures should not be valid anymore.

Tranche token metadata can be changed if needed. This is handled by PoolManager.updateTrancheTokenMetadata function. This function will change name field, which is part of _DOMAIN_SEPARATOR. The problem is that after changing the name field, _DOMAIN_SEPARATOR will not be recalculated, which means that all signatures that were signed before will be still valid. Also as i have described in another report, user's will see that Tranche token name is not same as in the domain separator that they should sign, so users will likely reject signing. And people who will craft messages themselves will use real updated name to create domain separator, so it will not match and permit will revert.

Tools Used

VsCode

Recommended Mitigation Steps

When you update name, then recalculate domain separator.

Assessed type

Error

QA-01. Messages._stringToBytes32 will truncate data for messages that are longer than 32 bytes

Details

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/gateway/Messages.sol#L876-L885

    function _stringToBytes32(string memory source) internal pure returns (bytes32 result) {
        bytes memory tempEmptyStringTest = bytes(source);
        if (tempEmptyStringTest.length == 0) {
            return 0x0;
        }


        assembly {
            result := mload(add(source, 32))
        }
    }

In order to convert string to bytes32, function just returns 32 bytes, But in case if string is bigger than 32 bytes, then it will truncate string and provide wrong data.

Recommended Mitigation Steps

Check that string is not bigger than 32 bytes.

QA-02. Users can call several messages a lot of times on cheap chains in order to flood centrifuge chain with messages.

Details

All messages that are relied to the centrifuge chain from Gateway are then executed by protocol's bot. This bot will pay gas fees for execution.

Users on cheap chains can call PoolManager.transferTrancheTokensToCentrifuge with 0 amount just to pass message on centrifuge chain. Also they can call LiquidityPool.collectDeposit and LiquidityPool.collectRedeem for a member. As result they can spam with big amount of messages, which bots should execute or skip.

Recommended Mitigation Steps

Do not allow bridge 0 tokens. Restrict LiquidityPool.collectDeposit and LiquidityPool.collectRedeem to owners only.

QA-03. increaseAllowance/decreaseAllowance may not work with some wallets.

Details

Recently, open zeppelin has remove increaseAllowance/decreaseAllowance function form ERC20 contract. This is because they are not described in the standard and as described in the disscussion such function may not work well with some wallets that are trying to limit user's activity(spending/approving).

But ERC20 of centrifuge protocol, still has such functions.

Recommended Mitigation Steps

Think, if it's worth removing them as well.

QA-04. ERC20._isValidSignature doesn't check if ecrecover returned 0 address

Details

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L196-L214

        function _isValidSignature(address signer, bytes32 digest, bytes memory signature) internal view returns (bool) {
        if (signature.length == 65) {
            bytes32 r;
            bytes32 s;
            uint8 v;
            assembly {
                r := mload(add(signature, 0x20))
                s := mload(add(signature, 0x40))
                v := byte(0, mload(add(signature, 0x60)))
            }
            if (signer == ecrecover(digest, v, r, s)) {
                return true;
            }
        }


        (bool success, bytes memory result) =
            signer.staticcall(abi.encodeWithSelector(IERC1271.isValidSignature.selector, digest, signature));
        return (success && result.length == 32 && abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector);
    }

This function checks signer and doesn't check if ecrecover returned 0 address. A result user can create allowance to himself from 0 address. This is not real problem as 0 addresss is restricted, so no one can send or mint tokens to it.

Recommended Mitigation Steps

In case if recovered address is 0, then revert.