Blur Exchange contest - Trust's results

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L212-L227

Vulnerability details

Description

remainingETH is an important state variable in Exchange.sol, which keeps track of how many ETH have yet to be used as payment from the current msg.value. The setupExecution modifier sets the value before and after execution:

modifier setupExecution() {
    remainingETH = msg.value;
    isInternal = true;
    _;
    remainingETH = 0;
    isInternal = false;
}

Payments are deducted from remainingETH:

function _executeFundsTransfer(
    address seller,
    address buyer,
    address paymentToken,
    Fee[] calldata fees,
    uint256 price
) internal {
    if (msg.sender == buyer && paymentToken == address(0)) {
        require(remainingETH >= price);
        remainingETH -= price;
    }
    /* Take fee. */
    uint256 receiveAmount = _transferFees(fees, paymentToken, buyer, price);
    /* Transfer remainder to seller. */
    _transferTo(paymentToken, buyer, seller, receiveAmount);
}

Another important note is that _returnDust() is executed in the end of execute() / bulkExecute() to send back to msg.sender the remaining ETH:

function _returnDust() private {
    uint256 _remainingETH = remainingETH;
    assembly {
        if gt(_remainingETH, 0) {
            let callStatus := call(
                gas(),
                caller(),
                selfbalance(),
                0,
                0,
                0,
                0
            )
        }
    }
}

The issue created here is that _returnDust passes execution to caller without updating remainingETH. Caller may then immediately enter _execute() and use the remainingETH sum as if it is still remaining. Interestly this is possible despite _execute being guarded by reentrancyGuard and internalCall. reentrancyGuard is bypassed because in the outer call we are in bulkExecute(), not _execute. internalCall is bypassed because while we are in the outer bulkExecute(), the setupExecution() modifier guarantees that isInternal is set to true. Therefore, in the latter _executeFundsTransfer remainingETH will be old instead of zero, and the require will succeed.

Impact

Attacker can spoof remainingETH and double-spend their input ETH to Exchange

Proof of Concept

The call chain would look like:

• bulkExecute()
  • _execute()
    • _executeFundsTransfer()
  • _returnDust()
    • caller.call()
      • _execute()
        • _executeFundsTransfer()

Tools Used

Manual audit

Recommended Mitigation Steps

Use the Checks / Effects / Interactions pattern: Before calling user in _returnDust(), update remainingETH to 0.

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L216

Vulnerability details

Description

_returnDust function is used to return unused ETH from the trade executions back to the sender. It is implemented like so:

function _returnDust() private {
    uint256 _remainingETH = remainingETH;
    assembly {
        if gt(_remainingETH, 0) {
            let callStatus := call(
                gas(),
                caller(),
                selfbalance(),
                0,
                0,
                0,
                0
            )
        }
    }
}

remainingETH keeps track of how many of user's ETH remain after trade executions. The issue is that the caller of execute() or bulkExecute() may be a contract that does not accept no-calldata calls (does not have a fallback function). In that event, the call() will fail and return 0 to the callStatus variable. However, since it is never checked, _returnDust will complete successfully and therefore leak the remaining ETH value.

Impact

ETH is leaked when execute() is called from a contract that doesn't have a fallback function.

Tools Used

Manual audit

Recommended Mitigation Steps

It is recommended to require that callStatus is true. It makes sense to add a flag dontForceRefund, which will override the default revert behavior.

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L639 https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L30

Vulnerability details

Description

In Non-Fungible's security model, users approve their ERC20 / ERC721 / ERC1155 tokens to the ExecutionDelegate contract, which accepts transfer requests from Exchange.

The requests are made here:

function _transferTo(
    address paymentToken,
    address from,
    address to,
    uint256 amount
) internal {
    if (amount == 0) {
        return;
    }
    if (paymentToken == address(0)) {
        /* Transfer funds in ETH. */
        require(to != address(0), "Transfer to zero address");
        (bool success,) = payable(to).call{value: amount}("");
        require(success, "ETH transfer failed");
    } else if (paymentToken == POOL) {
        /* Transfer Pool funds. */
        bool success = IPool(POOL).transferFrom(from, to, amount);
        require(success, "Pool transfer failed");
    } else if (paymentToken == WETH) {
        /* Transfer funds in WETH. */
        executionDelegate.transferERC20(WETH, from, to, amount);
    } else {
        revert("Invalid payment token");
    }
}

function _executeTokenTransfer(
    address collection,
    address from,
    address to,
    uint256 tokenId,
    uint256 amount,
    AssetType assetType
) internal {
    /* Call execution delegate. */
    if (assetType == AssetType.ERC721) {
        executionDelegate.transferERC721(collection, from, to, tokenId);
    } else if (assetType == AssetType.ERC1155) {
        executionDelegate.transferERC1155(collection, from, to, tokenId, amount);
    }
}

The issue is that there is a significant centralization risk trusting Exchange.sol contract to behave well, because it is an immediately upgradeable ERC1967Proxy. All it takes for a malicious owner or hacked owner to upgrade to the following contract:

function _stealTokens(
    address token,
    address from,
    address to,
    uint256 tokenId,
    uint256 amount,
    AssetType assetType
) external onlyOwner {
    /* Call execution delegate. */
    if (assetType == AssetType.ERC721) {
        executionDelegate.transferERC721(token, from, to, tokenId);
    } else if (assetType == AssetType.ERC1155) {
        executionDelegate.transferERC1155(token, from, to, tokenId, amount);
    } else if (assetType == AssetType.ERC20) {
        executionDelegate.transferERC20(token, from, to, amount);
}

At this point hacker or owner can steal all the assets approved to Non-Fungible.

Impact

Hacked owner or malicious owner can immediately steal all assets on the platform

Tools Used

Manual audit

Recommended Mitigation Steps

Exchange contract proxy should implement a timelock, to give users enough time to withdraw their approvals before some malicious action becomes possible.

Judging note

The status quo regarding significant centralization vectors has always been to award M severity, in order to warn users of the protocol of this category of risks. See here for list of centralization issues previously judged.

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L378

Vulnerability details

Description

The Non Fungible supplied docs state: Off-chain methods "Oracle cancellations - if the order is signed with an expirationTime of 0, a user can request an oracle to stop producing authorization signatures; without a recent signature, the order will not be able to be matched"

From the docs, we can expect that when expirationTime is 0 , trader wishes to enable dynamic oracle cancellations. However, the recent code refactoring broke not only this functionality but all orders with expirationTime = 0.

Previous _validateOrderParameters:

function _validateOrderParameters(Order calldata order, bytes32 orderHash)
    internal
    view
    returns (bool)
{
    return (
        /* Order must have a trader. */
        (order.trader != address(0)) &&
        /* Order must not be cancelled or filled. */
        (cancelledOrFilled[orderHash] == false) &&
        /* Order must be settleable. */
        _canSettleOrder(order.listingTime, order.expirationTime)
    );
}
/**
 * @dev Check if the order can be settled at the current timestamp
 * @param listingTime order listing time
 * @param expirationTime order expiration time
 */
function _canSettleOrder(uint256 listingTime, uint256 expirationTime)
    view
    internal
    returns (bool)
{
    return (listingTime < block.timestamp) && (expirationTime == 0 || block.timestamp < expirationTime);

New _validateOrderParameters:

function _validateOrderParameters(Order calldata order, bytes32 orderHash)
    internal
    view
    returns (bool)
{
    return (
        /* Order must have a trader. */
        (order.trader != address(0)) &&
        /* Order must not be cancelled or filled. */
        (!cancelledOrFilled[orderHash]) &&
        /* Order must be settleable. */
        (order.listingTime < block.timestamp) &&
        (block.timestamp < order.expirationTime)
    );
}

Note the requirements on expirationTime in _canSettleOrder (old) and _validateOrderParameters (new). If expirationTime == 0, the condition was satisfied without looking at block.timestamp < expirationTime. In the new code, block.timestamp < expirationTime is always required in order for the order to be valid. Clearly, block.timestamp < 0 will always be false, so all orders that wish to make use of off-chain cancellation will never execute.

Impact

All orders which use expirationTime == 0 to support oracle cancellation are not executable.

Tools Used

Manual audit, diffing tool

Recommended Mitigation Steps

Implement the checks the same way as they were in the previous version of Exchange.

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L13

Vulnerability details

Description

The docs state: "The pool allows user to predeposit ETH so that it can be used when a seller takes their bid. It uses an ERC1967 proxy pattern and only the exchange contract is permitted to make transfers."

Pool is designed as an ERC1967 upgradeable proxy which handles balances of users in Not Fungible. Users may interact via deposit and withdraw with the pool, and use the funds in it to pay for orders in the Exchange.

Pool is declared like so:

contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
	function _authorizeUpgrade(address) internal override onlyOwner {}
	...

Importantly, it has no constructor and no initializers. The issue is that when using upgradeable contracts, it is important to implement an initializer which will call the base contract's initializers in turn. See how this is done correctly in Exchange.sol:

/* Constructor (for ERC1967) */
function initialize(
    IExecutionDelegate _executionDelegate,
    IPolicyManager _policyManager,
    address _oracle,
    uint _blockRange
) external initializer {
    __Ownable_init();
    isOpen = 1;
	  ...
}

Since Pool skips the __Ownable_init initialization call, this logic is skipped:

function __Ownable_init() internal onlyInitializing {
    __Ownable_init_unchained();
}
function __Ownable_init_unchained() internal onlyInitializing {
    _transferOwnership(_msgSender());
}

Therefore, the contract owner stays zero initialized, and this means any use of onlyOwner will always revert.

The only use of onlyOwner in Pool is here:

function _authorizeUpgrade(address) internal override onlyOwner {}

The impact is that when the upgrade mechanism will check caller is authorized, it will revert. Therefore, the contract is unexpectedly unupgradeable. Whenever the EXCHANGE or SWAP address, or some functionality needs to be changed, it would not be possible.

Impact

The Pool contract is designed to be upgradeable but is actually not upgradeable

Proof of Concept

In the 'pool' test in execution.test.ts, add the following lines:

it('owner configured correctly', async () => {
  expect(await pool.owner()).to.be.equal(admin.address);
});

It shows that the pool after deployment has owner as 0x0000...00

Tools Used

Manual audit, hardhat

Recommended Mitigation Steps

Implement an initializer for Pool similarly to the Exchange.sol contract.