Skip to main content
Dedaub

Blur Finance - Exchange v1.0 Contracts

Smart Contract Security Assessment

June 10, 2022

Blur

SUMMARY


ABSTRACT

Dedaub was commissioned to perform an audit on Blur’s Exchange v1.0 contracts on commit hash 3701dede2c592a41a75abe3f4333fed99f75c33c. The audited contract list is the following:

ExchangeV1.0/Exchange.sol

FeeMechanism.sol

Registry.sol

registry/ProxyRegistry.sol

registry/AuthenticatedProxy.sol

registry/OwnableDelegateProxy.sol

registry/proxy/OwnedUpgradeabilityProxy.sol

registry/proxy/OwnedUpgradeabilityStorage.sol

registry/proxy/Proxy.sol

registry/TokenRecipient.sol

registry/TokenTransferProxy.sol

exchange/ERC1967.sol

token/gauges/GaugeStorage.vy

token/gauges/LiquidityGaugeFactory.vy

token/gauges/LiquidityGauge.vy

token/Minter.vy


Some external contracts in close interaction with the system were also inspected, in essence Open Zeppelin’s contracts UUPSUpgradeable and ERC1967UpgradeUpgradeable.

Two auditors worked on the codebase over two weeks.


SETTING & CAVEATS

The audited codebase is of moderate size of ~2KLoC.

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Functional correctness relative to low-level calculations (including units, scaling, quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.


ARCHITECTURE AND HIGH-LEVEL RECOMMENDATIONS

The Blur protocol is a Non-Fungible Token (NFT) exchange protocol. Registered traders are invited to sign and place their orders off-chain, on the web application of the Blur marketplace. Anyone can trigger the execution of a matching buy-sell orders pair, which is validated and committed on-chain.

Registered users are assigned a Proxy contract which is able to perform low-level call() or delegatecall() transactions on their behalf and is only possible to be accessed by its Owner (the registered user) and some whitelisted contracts. Per the current implementation, it is used to perform the NFT transfer on users’ behalf, upon a matching pair of orders. In any future extended functionality of the system, the Proxy, as the user’s representative, might be used to execute other on-chain transactions as well.
Any user is able to revoke this permission at any time. A registered user also has the ability to transfer access to her Proxy to any other address. It is not yet clear whether this access-transfer functionality is supposed to be used solely for a user to migrate her Proxy ownership from one of her addresses to another, or it is also meant to be used to trade proxies among different users. As we point out in issue L4, proxy trading through this mechanism would be problematic.

The protocol charges some fee upon each successful trade, essentially by withholding a feeRate on the buying price. The feeRate value is configurable and can be altered at any time by the Owner of the FeeMechanism contract, which is, in essence, the Blur team. Also, the Exchange contract, which contains the fundamental implementation of the exchange protocol’s logic, is upgradeable giving the ability to the Blur team to upgrade functionality or patch potential security threats. As is typical in a configurable and upgradeable system design, some trust is required that the team will intervene only in accordance to the users’ interest.

The protocol is accompanied by a Curve-like reward mechanism, where the rewards are denominated in Blur’s own ERC20 token. The implementation is based on Curve’s codebase, with modifications in the gauge contracts (GaugeStorage.vy, LiquidityGaugeFactory.vy, LiquidityGauge.vy) being, most importantly, utilizing Merkle Trees for passing and verifying the users’ balances as well as the reward-period specific information.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed”, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

H1

Access-control requirement is commented out allowing anyone to call the users’ proxies

H1HIGH

Access-control requirement is commented out allowing anyone to call the users’ proxies
resolved

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.

H2

Incorrect require() statements in FeeMechanism::transfer can lead to DoS when the owner tries to retrieve the fees.

H2HIGH

Incorrect require() statements in FeeMechanism::transfer can lead to DoS when the owner tries to retrieve the fees.
resolved

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.



MEDIUM SEVERITY

[No medium severity issues]


LOW SEVERITY

L1

FeeMechanism internal accounting not updated after fees are withdrawn.

L1LOW

FeeMechanism internal accounting not updated after fees are withdrawn.
resolved

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.

L2

ProxyRegistry::transferAccessTo susceptible to DoS because of ProxyRegistry::registerProxyFor.

L2LOW

ProxyRegistry::transferAccessTo susceptible to DoS because of ProxyRegistry::registerProxyFor.
resolved

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.

L3

Exchange implementation contract should (also) be initialized out of the proxy’s context

L3LOW

Exchange implementation contract should (also) be initialized out of the proxy’s context
info

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.

L4

ProxyRegistry::transferProxy should not be called by users to transfer their proxies to other users.

L4LOW

ProxyRegistry::transferProxy should not be called by users to transfer their proxies to other users.
resolved

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.



OTHER/ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Dead code

A1ADVISORY

Dead code
resolved

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.

A2

Ownership transfer functionality redundantly requires two separate steps

A2ADVISORY

Ownership transfer functionality redundantly requires two separate steps
dismissed

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.

A3

Compiler known issues

A3ADVISORY

Compiler known issues
info

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.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub