function removeRouter() in RouterFacet don't check that router has balance and don't transfer it, it just set router owner and recipient to 0x0 which can cause make router balance in danger or unavailable for router owner

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L289-L325

Vulnerability details

Impact

There are some security levels for router, like setting owner and recipient and when removeRouter() is called this values set to 0x0 and router address become vulnerable. contract should transfer router balance to recipient before removing it.

Proof of Concept

This is removeRouter() code:

  /**
   * @notice Used to remove routers that can transact crosschain
   * @param router Router address to remove
   */
  function removeRouter(address router) external onlyOwner {
    // Sanity check: not empty
    if (router == address(0)) revert RoutersFacet__removeRouter_routerEmpty();

    // Sanity check: needs removal
    if (!s.routerPermissionInfo.approvedRouters[router]) revert RoutersFacet__removeRouter_notAdded();

    // Update mapping
    s.routerPermissionInfo.approvedRouters[router] = false;

    // Emit event
    emit RouterRemoved(router, msg.sender);

    // Remove router owner
    address _owner = s.routerPermissionInfo.routerOwners[router];
    if (_owner != address(0)) {
      emit RouterOwnerAccepted(router, _owner, address(0));
      // delete routerOwners[router];
      s.routerPermissionInfo.routerOwners[router] = address(0);
    }

    // Remove router recipient
    address _recipient = s.routerPermissionInfo.routerRecipients[router];
    if (_recipient != address(0)) {
      emit RouterRecipientSet(router, _recipient, address(0));
      // delete routerRecipients[router];
      s.routerPermissionInfo.routerRecipients[router] = address(0);
    }

    // Clear any proposed ownership changes
    s.routerPermissionInfo.proposedRouterOwners[router] = address(0);
    s.routerPermissionInfo.proposedRouterTimestamp[router] = 0;
  }

As you can see there is no check for router balances and owner can set all the security variables for router address to 0x0 and router balance would be in danger. There may be other functionalities that could be broke by just removing router without checking them.

Tools Used

VIM

Recommended Mitigation Steps

check some states before removing router.

[jakekidd]

Duplicate of https://github.com/code-423n4/2022-06-connext-findings/issues/150