Centrifuge - bin2chen's results

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/gateway/routers/axelar/Router.sol#L44

Vulnerability details

Vulnerability details

In AxelarRouter.sol, we need to ensure the legitimacy of the execute() method execution, mainly through two methods

1. axelarGateway.validateContractCall () to validate if the command is approved or not
2. onlyCentrifugeChainOrigin() is used to validate that sourceChain sourceAddress is legal.

Let's look at the implementation of onlyCentrifugeChainOrigin()

    modifier onlyCentrifugeChainOrigin(string calldata sourceChain, string calldata sourceAddress) {        
@>      require(msg.sender == address(axelarGateway), "AxelarRouter/invalid-origin");
        require(
            keccak256(bytes(axelarCentrifugeChainId)) == keccak256(bytes(sourceChain)),
            "AxelarRouter/invalid-source-chain"
        );
        require(
            keccak256(bytes(axelarCentrifugeChainAddress)) == keccak256(bytes(sourceAddress)),
            "AxelarRouter/invalid-source-address"
        );
        _;
    }

The problem is that this restriction msg.sender == address(axelarGateway)

When we look at the official axelarGateway.sol contract, it doesn't provide any call external contract 'sexecute() method

so msg.sender cannot be axelarGateway, and the official example does not restrict msg.sender

the security of the command can be guaranteed by axelarGateway.validateContractCall(), sourceChain, sourceAddress.

there is no need to restrict msg.sender

axelarGateway code address https://github.com/axelarnetwork/axelar-cgp-solidity/blob/main/contracts/AxelarGateway.sol

can't find anything that calls router.execute()

Impact

router.execute() cannot be executed properly, resulting in commands from other chains not being executed， protocol not working properly

Recommended Mitigation

remove msg.sender restriction

    modifier onlyCentrifugeChainOrigin(string calldata sourceChain, string calldata sourceAddress) {        
-       require(msg.sender == address(axelarGateway), "AxelarRouter/invalid-origin");
        require(
            keccak256(bytes(axelarCentrifugeChainId)) == keccak256(bytes(sourceChain)),
            "AxelarRouter/invalid-source-chain"
        );
        require(
            keccak256(bytes(axelarCentrifugeChainAddress)) == keccak256(bytes(sourceAddress)),
            "AxelarRouter/invalid-source-address"
        );
        _;
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability details

In the constructor method of ERC20.SOL, _DOMAIN_SEPARATOR will be generated by _calculateDomainSeparator() and this variable will be used as part of the signature in permit().

The main method is as follows.

contract ERC20 is Context {
...
    constructor(uint8 decimals_) {
        decimals = decimals_;
        wards[_msgSender()] = 1;
        emit Rely(_msgSender());

        deploymentChainId = block.chainid;
@>      _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
    }

    function _calculateDomainSeparator(uint256 chainId) private view returns (bytes32) {
        return keccak256(
            abi.encode(
                keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
@>?             keccak256(bytes(name)),
                keccak256(bytes(version)),
                chainId,
                address(this)
            )
        );
    }

    function permit(address owner, address spender, uint256 value, uint256 deadline, bytes memory signature) public {
        require(block.timestamp <= deadline, "ERC20/permit-expired");
        require(owner != address(0), "ERC20/invalid-owner");

        uint256 nonce;
        unchecked {
            nonce = nonces[owner]++;
        }
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
@>              block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid),
                keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonce, deadline))
            )
        );

        require(_isValidSignature(owner, digest, signature), "ERC20/invalid-permit");

        allowance[owner][spender] = value;
        emit Approval(owner, spender, value);
    }

the content of _DOMAIN_SEPARATOR contains the token's name

The problem is that there are two issues with this token.name.

1. token.name is not initialized in the constructor, it needs to be assigned by file()

2. poolManager.updateTrancheTokenMetadata() can modify token.name later.

so _DOMAIN_SEPARATOR will contain a blank name , it's wrong.

Because name can change, the immutable nature of _DOMAIN_SEPARATOR is infeasible and it cannot be used as part of a signature.

It is recommended not to use _DOMAIN_SEPARATOR, but to calculate it in real-time with _calculateDomainSeparator().

Impact

The user assembles the content of the signature through the EIP-712 standard, but cannot execute permit() properly.

Proof of Concept

The following code demonstrates this, assembling the signature content as standard, but not the same as token.DOMAIN_SEPARATOR()

add to Tranche.t.sol

    function test_NoEqualDomainSeparator() public {
        bytes32 tokenDomainSeparator = token.DOMAIN_SEPARATOR();
        bytes32 calculateDomainSeparator = keccak256(
            abi.encode(
                keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
                keccak256(bytes(token.name())),
                keccak256(bytes(token.version())),
                block.chainid,
                address(token)
            )
        );
      
        console.log("equal:",tokenDomainSeparator == calculateDomainSeparator);
    }

 $ forge test -vvv --match-test test_NoEqualDomainSeparator

Running 1 test for test/token/Tranche.t.sol:TrancheTokenTest
[PASS] test_NoEqualDomainSeparator() (gas: 14966)
Logs:
  equal: false

Recommended Mitigation

remove _DOMAIN_SEPARATOR , calculate it in real-time with _calculateDomainSeparator().

contract ERC20 is Context {
...

-   uint256 public immutable deploymentChainId;
-   bytes32 private immutable _DOMAIN_SEPARATOR;

    constructor(uint8 decimals_) {
        decimals = decimals_;
        wards[_msgSender()] = 1;
        emit Rely(_msgSender());

-       deploymentChainId = block.chainid;
-       _DOMAIN_SEPARATOR = _calculateDomainSeparator(block.chainid);
    }


    function DOMAIN_SEPARATOR() external view returns (bytes32) {
-       return block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid);
+       return _calculateDomainSeparator(block.chainid);
    }

    function permit(address owner, address spender, uint256 value, uint256 deadline, bytes memory signature) public {
        require(block.timestamp <= deadline, "ERC20/permit-expired");
        require(owner != address(0), "ERC20/invalid-owner");

        uint256 nonce;
        unchecked {
            nonce = nonces[owner]++;
        }
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
-               block.chainid == deploymentChainId ? _DOMAIN_SEPARATOR : _calculateDomainSeparator(block.chainid),
+               _calculateDomainSeparator(block.chainid), 
                keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, nonce, deadline))
            )
        );

        require(_isValidSignature(owner, digest, signature), "ERC20/invalid-permit");

        allowance[owner][spender] = value;
        emit Approval(owner, spender, value);
    }

Assessed type

Context