I've competed in this contest between 11/11/22-14/11/22 and achieved first place. It was a review of several changes made after the first contest which run about a month prior. Blur is an NFT marketplace. Repo is here.
HIGH: 1
MED: 5
MED: Hacked owner or malicious owner can immediately steal all assets on the platform
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
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.
MED: Current code will not deploy successfully without refactoring
Description
In Exchange.sol, there are several consts defined:
string public constant NAME = "Exchange";
string public constant VERSION = "1.0";
uint256 public constant INVERSE_BASIS_POINT = 10_000;
address public constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
address public constant POOL = 0xF66CfDf074D2FFD6A4037be3A669Ed04380Aef2B;Similarly, Pool.sol also defines several consts:
address private constant EXCHANGE = 0x707531c9999AaeF9232C8FEfBA31FBa4cB78d84a;
// TODO: set proper address before deployment
address private constant SWAP = 0x707531c9999AaeF9232C8FEfBA31FBa4cB78d84a;The issue is that this introduces a constant reference loop between Pool and Exchange - a chicken or the egg problem. POOL Proxy address is only known after Pool is deployed, but Pool deployment requires Exchange Proxy's address, which requires Pool's address, ad infinitum.
Impact
Current code will not deploy successfully without refactoring
Recommended Mitigation Steps
In at least one of Pool / Exchange, make the complement's address writeable.
MED: All orders which use expirationTime == 0 to support oracle cancellation are not executable.
Explanation
The Non Fungible supplied docs state:
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.
Recommended Mitigation Steps
Implement the checks the same way as they were in the previous version of Exchange.
HIGH: Attacker can spoof remainingETH and double-spend their input ETH to Exchange
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()
Recommended Mitigation Steps
Use the Checks / Effects / Interactions pattern:
Before calling user in _returnDust(), update remainingETH to 0.
MED: Potential ETH leak of value when execute() is called from a contract
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.
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.
MED: Pool designed to be upgradeable but does not set owner, making it unupgradeable
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 below:
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
Recommended mitigation steps:
Implement an initializer for Pool similarly to the Exchange.sol contract.
Comments