Blur Finance - Exchange v1.0 Contracts

Contract AuthenticatedProxy is the implementation contract behind the users’ proxies. This contract implements function proxy(), which performs low level call or delegateCall on behalf of the proxy owner. Within the Blur system, this function is called to perform the final NFT transfer transaction. However, the access-control checks are currently commented out, in essence allowing anyone to be able to execute arbitrary transactions:

function proxy(address dest, HowToCall howToCall, bytes memory data)
    public
    returns (bool result)
{
    // Dedaub: it's crucial that this requirement is active
    // require(msg.sender == user || (!revoked && registry.contracts(msg.sender)), "Authenticated proxy can only be called by its user, or by a contract authorized by the registry as long as the user has not revoked access");
    bytes memory ret;
    if (howToCall == HowToCall.Call) {
        (result, ret) = dest.call(data);
    } else if (howToCall == HowToCall.DelegateCall) {
        (result, ret) = dest.delegatecall(data);
    }
    return result;
}

The team immediately confirmed that the require()ment was accidentally left commented out during some debugging tests and reassured that it will be restored.
The issue has been resolved in commit da76e990deb14b69643b13f0a2411a4243abb8f9.

The owner of the FeeMechanism contract can use the transfer function to retrieve fees held inside the contract. In order to do this, the owner specifies the token he wants to retrieve, the collection from which he wants to retrieve the token, and the amount to retrieve.

This input is validated through two require statements so as to check whether there are enough funds associated with the collection, and in the contract itself (as suggested by the error messages).

function transfer(address token, address collection, address to, uint amount)
    external
    onlyOwner
    reentrancyGuard
  {
    // Dedaub: these are the other way round

    require(balanceOf[collection] <= amount, "Insufficient collection balance");
    require(totalBalance[token] <= amount, "Insufficient token balance");

    if (token == address(0)) {
      require(to != address(0));
      (bool success,) = to.call{value: amount}("");
      require(success);
    } else {
      require(ERC20(token).transfer(to, amount));
    }
  }

This is done through the inequalities:

balanceOf[collection] <= amount
totalBalance[token] <= amount

However to effect these checks, these inequalities should be the other way around:

balanceOf[collection] >= amount
totalBalance[token] >= amount

Otherwise a potentially dangerous situation arises. For if the amount is strictly greater than totalBalance[token], then the subsequent call or transfer actions will fail. Hence the requested amount can only be exactly equal to totalBalance[token]. But, still, an attacker can front-run the call to FeeMechanism::transfer by depositing a small amount of WETH through FeeMechanism::transferFee. This changes totalBalance[token] so that it is not equal to amount, stopping the owner from extracting the fees from the FeeMechanism contract.

The team resolved this issue at commit hash da76e990deb14b69643b13f0a2411a4243abb8f9.

When the owner of FeeMechanism withdraws fees from the contract related to a specific collection using transfer, the amount is not subtracted from balanceOf[collection], and neither is it subtracted from totalBalance[token]. Therefore the owner can call transfer on the same collection again to retrieve more fees until the funds in the contract are exhausted and ERC20::transfer fails.
We suggest that the contract account for the withdrawn fees, both to keep within the collection design and to avoid future issues when new functionality depends on totalBalance[token] or balanceOf[collection].

The team resolved this issue by subtracting the withdrawn amount both from the collection’s and the token’s total balance, at commit hash da76e990deb14b69643b13f0a2411a4243abb8f9.

In ProxyRegistry, a user can use the function transferAccessTo to transfer his proxy to another address. However, this requires the recipient address to not already have a proxy.

function transferAccessTo(address from, address to)
        public
    {
        OwnableDelegateProxy proxy = proxies[from];

        /* CHECKS */
        require(msg.sender == address(proxy), "Proxy transfer can only be called by the proxy");
        require(address(proxies[to]) == address(0), "Proxy transfer has existing proxy as destination");

        /* EFFECTS */
        delete proxies[from];
        proxies[to] = proxy;
    }

Now, the registerProxyFor function of the ProxyRegistry contract can be called by any caller and can be used to gift a proxy to a particular address. Therefore an attacker can make the transferAccessTo call fail by front running it and gifting a proxy to the recipient address, leading to a DoS situation.

This can be solved by only allowing callers to create their own proxies.

The team resolved this issue by limiting the access to function ProxyRegistry::registerProxyFor from public to internal, at commit hash da76e990deb14b69643b13f0a2411a4243abb8f9.

Contract Exchange.sol contains all the protocol’s logic for the orders to be matched and executed. This contract is upgradeable through the UUPS proxy pattern, setting its owner during initialization. It is expected that the contract will be initialized inside the proxy’s context for the whole system to bootstrap. However, it is crucial that it is also dummy-initialized in its own context as well, so as to eliminate a well-known attack vector of the UUPS proxy, where an attacker initializes the implementation contract making herself the owner and then forces it to selfdestruct using the specifics of the upgradeability mechanism.

Since this separate initialization step cannot be guaranteed by the solidity codebase, we make this note to make sure it is not going to be missed.

ProxyRegistry::transferProxy should be strictly used to only transfer proxies from a user’s address to another one of the same user’s addresses.

It should not be used to transfer proxies from an address owned by one user to another address owned by another user.

The reason for this is that the initial user keeps significant power over the proxy after the transfer has taken place. More specifically, the privileged user address of the Authenticated proxy is not updated by transferProxy. Hence, the original address can call setRevoke and disable the new owner’s ability to call the proxy function. The original address can also continue calling the proxy function, potentially draining the AuthenticatedProxy of assets after it has been transferred.

The team has confirmed that the primary use case of ProxyRegistry::transferProxy is to transfer proxies from a user’s address to another one of the user’s addresses.

The team clearly documented that it should not be used for transferring ownership of the proxy between users at commit hash ​​da76e990deb14b69643b13f0a2411a4243abb8f9.

The are some dead code in lines LiquidityGauge.vy as follows:

// Dedaub: constant state variables declared but never used
TOKENLESS_PRODUCTION: constant(uint256) = 40
BOOST_WARMUP: constant(uint256) = 2 * 7 * 86400

// Dedaub: state variables declared and initialized in initialize() but never used
collection: public(address)
voting_escrow: public(address)

The team resolved this issue by removing the unused state variables, at commit hash da76e990deb14b69643b13f0a2411a4243abb8f9.

In contracts LiquidityGauge.vy and LiquidityGaugeFactory.vy ownership is transferred after the current admin calls first the commit_transfer_ownership() and then the apply_transfer_ownership() functions. However, there seems to be no benefit in splitting ownership transfer action in two separate transactions, since neither a timelock period is required between committing and applying nor is the future admin required to be the one who applies the transfer:

@external
def commit_transfer_ownership(addr: address) -> bool:
    """
    @notice Transfer ownership of GaugeController to `addr`
    @param addr Address to have ownership transferred to
    """
    assert msg.sender == self.admin  # dev: admin only
    self.future_admin = addr
    log CommitOwnership(addr)

    return True


@external
def apply_transfer_ownership() -> bool:
    """
    @notice Apply pending ownership transfer
    """
    // Dedaub: the following two commands should be in the reverse order if the future admin is supposed to "accept" the ownership
    assert msg.sender == self.admin  # dev: admin only
    _admin: address = self.future_admin
    assert _admin != ZERO_ADDRESS  # dev: admin not set
    self.admin = _admin
    log ApplyOwnership(_admin)

    return True

We suggest either merging these two steps into one single ownership-transfer step, so as to reduce gas costs, or require the future admin “accepts” the ownership within the apply step.

The team decided to dismiss this suggestion, as the ownership transfer was designed to be managed by a governance system which itself can have an execution delay, thus the two-step process grants the DAO flexibility.

Solidity compiler version v0.8.13 has, at the time of writing, some known bugs. We inspected the code and found that it is not affected by these bugs.