LMCV part 3 - DAMfinance

It was identified that the dPrimeConnectorHyperlane contract was incompatible with the latest Hyperlane protocol and might not work with new versions of Hyperlane Inbox contracts, which can lead to users losing their dPrime tokens during cross-chain transfers.

The dPrimeConnectorHyperlane contract is inherited from the Hyperlane Router abstract contract. When receiving a message from another chain, the handle function is called by the corresponding Hyperlane Inbox contract. This function calls the internal _handle function that can be overridden in the derived contracts.

The following code is an example from the Hyperlane Router version 0.5.0:

@hyperlane-xyz/app/contracts/Router.sol (0.5.0)

    function handle(
        uint32 _origin,
        bytes32 _sender,
        bytes memory _message
    ) external virtual override onlyInbox onlyRemoteRouter(_origin, _sender) {
        // TODO: callbacks on success/failure
        _handle(_origin, _sender, _message);
    }

    // ============ Virtual functions ============
    function _handle(
        uint32 _origin,
        bytes32 _sender,
        bytes memory _message
    ) internal virtual;

The _handle function was overridden in the dPrimeConnectorHyperlane contract:

hyperlane/dPrimeConnectorHyperlane.sol

    function _handle(
        uint32 _origin,
        bytes32,
        bytes memory _message
    ) internal override alive {

However, from Hyperlane version 0.5.1, the storage location of the _message parameter was changed from memory to calldata:

@hyperlane-xyz/app/contracts/Router.sol (0.5.1)

    function handle(
        uint32 _origin,
        bytes32 _sender,
        bytes calldata _message
    ) external virtual override onlyInbox onlyRemoteRouter(_origin, _sender) {
        // TODO: callbacks on success/failure
        _handle(_origin, _sender, _message);
    }

    // ============ Virtual functions ============
    function _handle(
        uint32 _origin,
        bytes32 _sender,
        bytes calldata _message
    ) internal virtual;

A bug concerning data location during inheritance was identified in Solidity on May 17, 2022. According to the Solidity team, the calldata pointer in these cases is interpreted as a memory pointer which results in reading invalid data from memory.

It is also noted that the current protocol is not configured with a fixed version of Hyperlane dependencies. In the package.json configuration file, floating versions are used, which means that the contract might be deployed with an incompatible Hyperlane messaging protocol:

package.json

  "dependencies": {
    "@chainlink/contracts": "^0.4.1",
    "@hyperlane-xyz/app": "^0.5.0",
    "@hyperlane-xyz/sdk": "^0.5.0",
    "@layerzerolabs/solidity-examples": "^0.0.4",
    "@openzeppelin/contracts": "^4.5.0",
    "solc": "^0.8.15"
  }

As proof of concept, the dPrimeConnectorHyperlane contract was deployed with Hyperlane version 0.5.0. It was possible to mint tokens by calling the handle function:

The contract was also deployed using the Hyperlane version 0.5.1. Calling the handle function with the same parameters resulted in an error and corrupted data:

Note that it is not possible to recover the tokens from corrupted data using the retry function of the dPrimeConnectorHyperlane contract.

SOLVED: The DAMfinance team solved the issue in commit c19b217 by changing the storage location of the _message parameter from memory to calldata and fixing the versions of the Hyperlane contracts in the package.json configuration file.

The new version of the dPrime token contract was extended with an additional security feature that prevents transferring funds for a certain number of blocks after a cross-chain transfer. The delay allows the DAMfinance team team to pause the contract in case the cross-chain messaging is compromised to limit the impact of such security incidents.

However, a malicious actor can also exploit this behavior to manipulate the market during volatile times or perform a griefing attack by continuously sending tokens to the targeted users. The likelihood of such attacks is increased because it is enough to send only one token to prevent the transfer of the targeted user's funds temporarily.

Code Location

dPrime.sol

    function mintAndDelay(address to, uint256 value) external auth {
        transferBlockRelease[to] = block.number + transferBlockWait;
        _mint(to, value);
    }

dPrime.sol

    function transferFrom(address from, address to, uint256 value) external alive returns (bool) {
        require(to != address(0) && to != address(this), "dPrime/invalid-address");
        require(block.number > transferBlockRelease[from], "dPrime/transfer too soon after cross-chain mint");

SOLVED: The DAMfinance team solved the issue in commit e76f394 by adding a configurable threshold to the mintAndDelay function.

It was identified that in the dPrimeGuardian contract, it is not possible to add any values to the pipeAddresses mapping. And therefore, in case of an incident, the dPrimeGuardian contract cannot be used to take back the admin privileges of any insecure connector from the dPrime token because the removeConnectorAdmin reads the required addresses from the mapping.

Code Location

dPrimeGuardian.sol

function removeConnectorAdmin(bytes32 pipeName) external auth {
    dPrimeLike(dPrimeContract).deny(pipeAddresses[pipeName]);
    emit HaltedPipe(pipeName);
}

SOLVED: The DAMfinance team solved the issue in commit cfc13a8 by adding the setPipeAddress function to the dPrimeGuardian contract.

The connector and guardian contracts are inherited from the AuthAdmin contract. It was identified that in this contract, the cage function did not emit any events. As a result, blockchain monitoring systems might not be able to timely detect suspicious behaviors related to the cage function.

Note that the Cage event declared in the dPrimeConnectorHyperlane contract is not used and could be removed from the contract.

SOLVED: The DAMfinance team solved the issue in commit cfc13a8 by emitting the event in the cage function of the AuthAdmin contract and removing the redundant Cage event from the dPrimeConnectorHyperlane contract.

It was identified that within the code, several functions were lacking zero address or zero value validation on important parameters. Setting invalid parameters in the examples below might result in loss of funds, waste of gas, lose of administrative controls, or reverting during important operations:

Missing zero address checks:

LayerZero/dPrimeConnectorLZ.sol:

• Line 24 constructor is missing zero-address checks for _lzEndpoint, _dPrimeContract.

hyperlane/dPrimeConnectorHyperlane.sol:

• Line 71 initialize is missing zero-address checks for _abacusConnectionManager, _interchainGasPaymaster, dPrimeContract.
• Line 95 transferRemote is missing zero-address check for _recipient.

dPrimeGuardian.sol

• Line 16: constructor is missing zero-address check for _dPrimeContract.

It is also recommended to validate the following parameters and variables:

hyperlane/dPrimeConnectorHyperlane.sol:

• Line 95 transferRemote is missing zero value checks for _destination, _amount.
• Line 142 retry is missing zero-value check for amount.

SOLVED: The DAMfinance team solved the issue in commit cfc13a8 by adding checks where appropriate.

Shortening the revert strings to fit within 32 bytes will decrease deployment time gas and reduce runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead to calculate memory offset, etc.

ACKNOWLEDGED: The DAMfinance team acknowledged this finding.

State variables can be declared as constant or immutable. In both cases, the variables cannot be modified after the contract has been constructed. For constant variables, the value has to be fixed at compile-time, while for immutable, it can still be assigned at construction time.

The following state variables are missing the immutable modifier:

dPrimeGuardian.sol

• Line 16: address public dPrimeContract;

dPrimeConnectorLZ.sol

• Line 19: address public dPrimeContract;

SOLVED: The DAMfinance team solved the issue in commits e1a2d28 and cfc13a8 by adding the immutable modifier to the state variables.