Blur Exchange contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

The dust ETH refund may failed slient.

The delegate call may fail sliently.

Proof of Concept

The dust ETH refund logic does not handle the return value.

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

as we see, the return value callStatus is not handled. If the underlying sending is a smart contract and does not implement the default receive payable() function, the ETH refund will fail sliently. Then the user lost the dusted ETH and next person trade get the previous user's dusted ETH.

Same case is in the delegatecall when bulk executing.

mstore(memPointer, 0xe04d94ae00000000000000000000000000000000000000000000000000000000) // _execute
calldatacopy(add(0x04, memPointer), order_pointer, size)
// must be put in separate transaction to bypass failed executions
// must be put in delegatecall to maintain the authorization from the caller
let result := delegatecall(gas(), address(), memPointer, add(size, 0x04), 0, 0)

the result is not handled, allowing the delegateCall to fail sliently.

User may wrongly assume that the transaction succeed, but the transaction failed sliently.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project handle the return value for dusted ETH refund and for delegate call return value properly. When the return value is false, strictly revert the transaction.

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L633 https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L635 https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L58 https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L62

Vulnerability details

Impact

Admin can steal all the fund in the Pool.

Proof of Concept

The Pool contract is meant to be used as a liqudity buffer. so when the trade is setteled, the user can deposit the fund into the Pool beforehand to settle the trade.

else if (paymentToken == POOL) {
	/* Transfer Pool funds. */
	bool success = IPool(POOL).transferFrom(from, to, amount);
	require(success, "Pool transfer failed");
} 

Which calls

function transferFrom(address from, address to, uint256 amount)
	public
	returns (bool)
{
	if (msg.sender != EXCHANGE && msg.sender != SWAP) {
		revert('Caller is not authorized to transfer');
	}
	_transfer(from, to, amount);

	return true;
}

the internal function just substract balance from address(from) and add the balance to address(to)

function _transfer(address from, address to, uint256 amount) private {
	require(_balances[from] >= amount);
	require(to != address(0));
	_balances[from] -= amount;
	_balances[to] += amount;

	emit Transfer(from, to, amount);
}

let us look into the transfserFrom again:

function transferFrom(address from, address to, uint256 amount)
	public
	returns (bool)
{
	if (msg.sender != EXCHANGE && msg.sender != SWAP) {
		revert('Caller is not authorized to transfer');
	}
	_transfer(from, to, amount);

	return true;
}

note the access control:

	if (msg.sender != EXCHANGE && msg.sender != SWAP) {
		revert('Caller is not authorized to transfer');
	}

address SWAP is out of scope, but the address EXCHANGE is clearly the Exchange.sol set in the Pool.sol constructor.

Basically we can say the contract Exchange has the privilege to control the fund in the Pool because it can call transferFrom to modify the balance before account.

And the Exchange contract is upgradeable and admin can upgradeable.

This means a compromised admin can upgrade the Exchange to a malicious implementation and the malicious implementation can call TransferFrom directly to decrease the user's balance and increase the admin's balance, then admin can call

function withdraw(uint256 amount) public {
	require(_balances[msg.sender] >= amount);
	_balances[msg.sender] -= amount;
	(bool success,) = payable(msg.sender).call{value: amount}("");
	require(success);
	emit Transfer(address(0), msg.sender, amount);
}

to withdraw the fund from the Pool.sol at the cost of the user.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend recommend the admin use M of N mulsig to safeguard the admin address. Also the project can make sure that the function transferFrom

function transferFrom(address from, address to, uint256 amount)
	public
	returns (bool)
{
	if (msg.sender != EXCHANGE && msg.sender != SWAP) {
		revert('Caller is not authorized to transfer');
	}
	_transfer(from, to, amount);

	return true;
}

is only callable inside the function _transferTo in Exchange.sol when settling the payment.

function _transferTo(
	address paymentToken,
	address from,
	address to,
	uint256 amount
) internal {

[L-01] No Use of Upgradeable ReentrancyGuarded && EIP712 Contracts

Line Of Code

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

Vulnerability detail and recommended fix.

Exchange.sol makes use of Open Zeppelin's OwnableUpgradeable.sol && UUPSUpgradeable.sol in the file but does not use an upgradeable version of ReentrancyGuarded.sol && EIP712.sol Contracts.

Consider just using Open Zeppelin's upgradeable equivalent contracts or taking from them what is needed to make your version of the contracts upgradeable.

[L-02] No Storage Gap for Upgradeable Contracts

Line Of Code

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

Vulnerability detail and recommended fix.

For upgradeable contracts, inheriting contracts may introduce new variables. In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable acontract.

If no storage gap is added, when the upgradable contract introduces new variables, it may override the variables in the inheriting contract.

Consider adding a storage gap at the end of the upgradeable abstract contract

uint256[50] private __gap;

[L-03] listingTime can be spoofed in the order, then maker order can be executed as taker and taker order can be treated as maker

Line Of Code

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

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

Vulnerability detail and recommended fix.

whether order is taker order or makter order is determined by the listing time, which is in the control of the user,

function _canMatchOrders(Order calldata sell, Order calldata buy)
	internal
	view
	returns (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType)
{
	bool canMatch;
	if (sell.listingTime <= buy.listingTime) {
		/* Seller is maker. */
		require(policyManager.isPolicyWhitelisted(sell.matchingPolicy), "Policy is not whitelisted");
		(canMatch, price, tokenId, amount, assetType) = IMatchingPolicy(sell.matchingPolicy).canMatchMakerAsk(sell, buy);
	} else {
		/* Buyer is maker. */
		require(policyManager.isPolicyWhitelisted(buy.matchingPolicy), "Policy is not whitelisted");
		(canMatch, price, tokenId, amount, assetType) = IMatchingPolicy(buy.matchingPolicy).canMatchMakerBid(buy, sell);
	}
	require(canMatch, "Orders cannot be matched");

	return (price, tokenId, amount, assetType);
}

listingTime can be spoofed in the order, then maker order can be executed as taker and taker order can be treated as maker.

We recommend the exchange assign listing time strictly offline instead of letting user spoof the listing time.

[L-04] Outdated openzepplein version.

Line Of Code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/package.json#L61

Vulnerability detail and recommended fix.

the current openzeppelin version is oudated.

"@openzeppelin/contracts": "4.4.1",
"@openzeppelin/contracts-upgradeable": "^4.6.0",

Please update the openzepplein package to latest version.