[WP-H1] `Owner` of `OwnableFactoryHandler` can impersonate `NestedFactory` and call `onlyFactory` methods and steal funds from the protocol

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/NestedReserve.sol#L10-L24

Vulnerability details

There are multiple contracts that inherited OwnableFactoryHandler, which allows the Owner of these contracts to add an arbitrary address as factory, effective immediately.

Therefore, a malicious/compromised Owner will be able to call all the onlyFactory methods.

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/NestedReserve.sol#L10-L24

contract NestedReserve is OwnableFactoryHandler {
  using SafeERC20 for IERC20;

  /// @notice Release funds to a recipient
  /// @param _recipient The receiver
  /// @param _token The token to transfer
  /// @param _amount The amount to transfer
  function transfer(
      address _recipient,
      IERC20 _token,
      uint256 _amount
  ) external onlyFactory {
      require(_recipient != address(0), "NRS: INVALID_ADDRESS");
      _token.safeTransfer(_recipient, _amount);
  }

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/abstracts/OwnableFactoryHandler.sol#L27-L31

function addFactory(address _factory) external onlyOwner {
    require(_factory != address(0), "OFH: INVALID_ADDRESS");
    supportedFactories[_factory] = true;
    emit FactoryAdded(_factory);
}

PoC

Given:

• 1000e8 WBTC is stored in NestedReserve.

A malicious/compromised Owner of NestedReserve can do the following to steal funds from NestedReserve.

1. Call NestedReserve.sol#addFactory() and set HackerAddress as factory;
2. Call NestedReserve.sol#transfer() with:

• _recipient: HackerAddress
• _token: WBTC
• _amount: 1000e8

As a result, 1000e8 WBTC is stolen by the attacker.

A similar attack can be initiated on NestedRecords by calling:

NestedRecords#addFactory() -> NestedRecords#store() -> NestedFactory#withdraw().

Recommendation

1. Consider using a multi-sig for the Owner of all OwnableFactoryHandler contracts;
2. addFactory() should be timelocked;

[maximebrugel]

Mentioned in the readme file :

Some functions of the protocol require admin rights (onlyOwner).

The contracts are owned by the [TimelockController] (https://docs.openzeppelin.com/contracts/4.x/api/governance#TimelockController) contract from OpenZeppelin, set with a >7-days delay. This ensures the community has time to review any changes made to the protocol.

The owner of the TimelockController is a three-party multisignature wallet.

During the next phase of the protocol, the ownership will be transferred to a fully decentralized DAO.

[harleythedogC4]

Agree with sponsor - the recommendations the warden described to fix this issue are already in place, so this issue is not valid.