fund lose because of wrong transferId calculation in protocol contracts in different domains because of not synced storage values

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L702-L722

Vulnerability details

Impact

_getTransferId() code calculate transferId based on user specified parameters and canonical token which is retrieved from contract storage, and calculation most be exactly same for different domains for protocol logic to work properly. but if protocol storage in different domains had different values then transferId would be different and users would lose fund. the different storage could be because of mistake or in the process of supporting new tokens in domains.

Proof of Concept

This is _getTransferId() code:

  /**
   * @notice Calculates a transferId based on `execute` arguments
   * @dev Need this to prevent stack too deep
   */
  function _getTransferId(ExecuteArgs calldata _args) private view returns (bytes32) {
    (uint32 tokenDomain, bytes32 tokenId) = s.tokenRegistry.getTokenId(_args.local);
    return _calculateTransferId(_args.params, _args.amount, _args.nonce, tokenId, tokenDomain, _args.originSender);
  }

  /**
   * @notice Calculates a transferId based on `xcall` arguments
   * @dev Need this to prevent stack too deep
   */
  function _calculateTransferId(
    CallParams calldata _params,
    uint256 _amount,
    uint256 _nonce,
    bytes32 _canonicalId,
    uint32 _canonicalDomain,
    address _originSender
  ) private pure returns (bytes32) {
    return keccak256(abi.encode(_nonce, _params, _originSender, _canonicalId, _canonicalDomain, _amount));
  }

As you can see _canonicalId and _canonicalDomain are used to calculate transferId and they are retrieved with function tokenRegistry.getTokenId() which reads information from representationToCanonical[] storage value.

  function getTokenId(address _token) external view override returns (uint32 _domain, bytes32 _id) {
    ConnextMessage.TokenId memory _tokenId = representationToCanonical[_token];
    if (_tokenId.domain == 0) {
      _domain = _localDomain();
      _id = TypeCasts.addressToBytes32(_token);
    } else {
      _domain = _tokenId.domain;
      _id = _tokenId.id;
    }
  }

So if the storage variable representationToCanonical[] had different values in protocol's contract in different domains then transferId would be calculated differently for them and all those logics based on transferId would fail and users who used protocol would lose funds because they couldn't receive funds they sent in destination domain.

and in xcall() code uses adoptedToCanonical[] to find canonical token information which the values are stored in setupAsset() by owner. so if owner set's wrong values then xcall() would calculate different transferId

  function setupAsset(
    ConnextMessage.TokenId calldata _canonical,
    address _adoptedAssetId,
    address _stableSwapPool
  ) external onlyOwner {
    // Sanity check: needs approval
    if (s.approvedAssets[_canonical.id]) revert AssetFacet__addAssetId_alreadyAdded();

    // Update approved assets mapping
    s.approvedAssets[_canonical.id] = true;

    address supported = _adoptedAssetId == address(0) ? address(s.wrapper) : _adoptedAssetId;

    // Update the adopted mapping
    s.adoptedToCanonical[supported].domain = _canonical.domain;
    s.adoptedToCanonical[supported].id = _canonical.id;

    // Update the canonical mapping
    s.canonicalToAdopted[_canonical.id] = supported;

    // Emit event
    emit AssetAdded(_canonical.id, _canonical.domain, _adoptedAssetId, supported, msg.sender);

    // Add the swap pool
    _addStableSwapPool(_canonical, _stableSwapPool);
  }

Tools Used

VIM

Recommended Mitigation Steps

add some mechanism to make sure that transferId is calculated based on same storage values.

[LayneHaber]

connext/monorepo@f41a156

[0xleastwood]

connext/nxtp@f41a156

I'm not sure if this is the issue the warden raised. I think the correct issue related to this fix is in #222. This issue seems to be related to a mis-match in _canonicalId and _canonicalDomain values which are used to calculate transferId. This seems like it would be incredibly difficult to enforce as there is no way to verify that the source and destination chains have both been configured correctly. I'm actually inclined to downgrade this to QA and keep #222 open as high.

[LayneHaber]

Right -- I think that makes sense.

One thing to note: re canonical id and domain is for them to be misconfigured, the nomad message would likely have to be fraudulent. The canonical information is passed through the message sent via the BridgeRouter, and it is derived from checking the registry to see if it has a record of deploying a mad version of that token.

[0xleastwood]

Right -- I think that makes sense.

One thing to note: re canonical id and domain is for them to be misconfigured, the nomad message would likely have to be fraudulent. The canonical information is passed through the message sent via the BridgeRouter, and it is derived from checking the registry to see if it has a record of deploying a mad version of that token.

Couldn't tokenRegistry.getTokenId be vulnerable to containing the wrong information in the representationToCanonical mapping? This would impact how _getTransferId calculates transferId, but it should not prevent message execution on the destination chain because this ID is still unique. However, I think it would be potentially bad if the transfer was reconciled prior to execution, mainly because the transfer will be marked as reconciled using a the source domain's transferId. This could be another type of double spend attack. @LayneHaber

[0xleastwood]

Merging with #231.

[LayneHaber]

so the TokenRegistry.getTokenId seen here checks the mapping. the mapping would only have information there if a mad* version of that asset was deployed (or admin called enrollCustom), seen here.

the asset should only be deployed during ensureLocal, called once when the message was received. i'm not sure how there could be incorrect information there unless the admin function was called mistakenly. maybe im missing something?