Zer0 - zAuction

Description

Consider using the best type available in the function arguments and declarations instead of accepting address and later casting it to the correct type.

Examples

This is only one of many examples.

zAuction/contracts/zAuction.sol:L22-L26

function init(address accountantaddress) external {
    require(!initialized);
    initialized = true;
    accountant = zAuctionAccountant(accountantaddress);
}

zAuction/contracts/zAuction.sol:L52-L54

IERC721 nftcontract = IERC721(nftaddress);
weth.transferFrom(bidder, msg.sender, bid);
nftcontract.transferFrom(msg.sender, bidder, tokenid);

zAuction/contracts/zAuction.sol:L40-L42

IERC721 nftcontract = IERC721(nftaddress);
accountant.Exchange(bidder, msg.sender, bid);
nftcontract.transferFrom(msg.sender, bidder, tokenid);

zAuction/contracts/zAuctionAccountant.sol:L60-L63

function SetZauction(address zauctionaddress) external onlyAdmin{
    zauction = zauctionaddress;
    emit ZauctionSet(zauctionaddress);
}

Description

Domains are minted using safeMint which checks that the recipient accepts the token if it is a contract. This is basically to avoid that tokens get locked up in contracts by accident.

When transferring the token the system is not using the safeTransferFrom method which would perform the same checks. this appears to be inconsistent as creating a domain would check this condition, but transferring the token won’t.

This may be a design decision, however, it is recommended to document the rationale behind when checks are to be performed. It should be noted that token retrieval can be rejected by the recipient when using the safe* method family.

Examples

zNS/contracts/Registrar.sol:L257-L264

function _createDomain(
  uint256 domainId,
  address domainOwner,
  address minter,
  address controller
) internal {
  // Create the NFT and register the domain data
  _safeMint(domainOwner, domainId);

zNS/contracts/BasicController.sol:L71-L71

registrar.transferFrom(controller, owner, id);

zNS/contracts/StakingController.sol:L140-L140

registrar.transferFrom(controller, recoveredBidder, id);

zAuction/contracts/zAuction.sol:L42-L42

nftcontract.transferFrom(msg.sender, bidder, tokenid);

Description

zAuctionAccountant is essentially implementing a simplified version of WETH raising the question of whether it is worth re-creating a variant of WETH which may just be increasing the overall complexity of the system.

One way to tackle this would be to provide a method that accepts ETH bids that automatically wraps submissions to WETH or only allow WETH deposits/approvals.

Description

Consider moving ERC721TestToken to a dedicated test subdirectory.

zAuction/contracts/ERC721TestToken.sol:L28-L28

contract ERC721TestToken is Context, AccessControlEnumerable, ERC721Enumerable, ERC721Burnable, ERC721Pausable {

Description

It is considered best practice to reference external contracts that are commonly available via an integrity checked dependency and package management system from that system instead of copying contracts manually into the projects source directory.

see zAuction/contracts/oz. Consider importing the contracts via npm.

Description

Declaring SafeMath usingFor uint256 may improve code readability.

ethbalance[msg.sender] = SafeMath.add(ethbalance[msg.sender], msg.value);

to

ethbalance[msg.sender] = ethbalance[msg.sender].add(msg.value)

Examples

zAuction/contracts/zAuctionAccountant.sol:L34-L34

ethbalance[msg.sender] = SafeMath.add(ethbalance[msg.sender], msg.value);

Description

Consider following the official solidity code style guidelines. Consider enforcing code style guidelines in the CI pipeline using style checker/linter tools.

Examples

• the initialization function is named initialize in zNS/openzeppelin but init in zAuction

zAuction/contracts/zAuction.sol:L22-L22

function init(address accountantaddress) external {

• Methods in zAuctionAccountant are written in PascalCase which makes them less easily distinguishable from Events/Contracts.

zAuction/contracts/zAuctionAccountant.sol:L33-L33

function Deposit() external payable {

• lowercase starting events may be hard to distinguish from function calls

zAuction/contracts/zAuctionAccountant.sol:L14-L16

event zDeposited(address indexed depositer, uint256 amount);
event zWithdrew(address indexed withrawer, uint256 amount);
event zExchanged(address indexed from, address indexed to, uint256 amount);

• adhere to token contract naming convention: Exchange() -> transfer()

zAuction/contracts/zAuctionAccountant.sol:L54-L58

function Exchange(address from, address to, uint256 amount) external onlyZauction {
    ethbalance[from] = SafeMath.sub(ethbalance[from], amount);
    ethbalance[to] = SafeMath.add(ethbalance[to], amount);
    emit zExchanged(from, to, amount);
}

Description

The code generally does not appear to be production-ready. The methods zWithdraw and zDeposit do not appear to be properly implemented. zWithdraw rather burns ETH balance than withdrawing it for an account (missing transfer) and zDeposit manipulates an accounts balance but never receives the ETH amount it credits to an account.

Examples

zAuction/contracts/zAuctionAccountant.sol:L44-L52

    function zDeposit(address to) external payable onlyZauction {
        ethbalance[to] = SafeMath.add(ethbalance[to], msg.value);
        emit zDeposited(to, msg.value);
    }

    function zWithdraw(address from, uint256 amount) external onlyZauction {
        ethbalance[from] = SafeMath.sub(ethbalance[from], amount);
        emit zWithdrew(from, amount);
    }

Recommendation

The methods do not seem to be used by the zAuction contract. It is highly discouraged from shipping incomplete implementations in productive code. Remove dead/unreachable code. Fix the implementations to perform proper accounting before reintroducing them if they are called by zAuction.

Description

An administrator of zAuctionAccountant contract can update the zAuction contract without warning. This has the potential to violate a security goal of the system.

Specifically, privileged roles could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to the unfortunate timing of changes.

In general users of the system should have assurances about the behavior of the action they’re about to take.

Examples

• updating the zAuction takes effect immediately. This has the potential to fail acceptance of bids by sellers on the now outdated zAuction contract as interaction with the accountant contract is now rejected. This forces bidders to reissue their bids in order for the seller to be able to accept them using the Accountant contract. This may also be used by admins to selectively censor the acceptance of accountant based bids by changing the active zAuction address.

zAuction/contracts/zAuctionAccountant.sol:L60-L68

    function SetZauction(address zauctionaddress) external onlyAdmin{
        zauction = zauctionaddress;
        emit ZauctionSet(zauctionaddress);
    }

    function SetAdmin(address newadmin) external onlyAdmin{
        admin = newadmin;
        emit AdminSet(msg.sender, newadmin);
    }

• Upgradeable contracts may introduce the same unpredictability issues where the proxyUpgradeable owner may divert execution to a new zNS registrar implementation selectively for certain transactions or without prior notice to users.

Recommendation

The underlying issue is that users of the system can’t be sure what the behavior of a function call will be, and this is because the behavior can change at any time.

We recommend giving the user advance notice of changes with a time lock. For example, make all system-parameter and upgrades require two steps with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period. This allows users that do not accept the change to withdraw immediately.

Validate arguments before updating contract addresses (at least != current/0x0). Consider implementing a 2-step admin ownership transfer (transfer+accept) to avoid losing control of the contract by providing the wrong ETH address.

Description

The lifecycle of a bid both for zAuction and zNS is not clear, and has many flaws.

• zAuction - Consider the case where a bid is placed, then the underlying asset in being transferred to a new owner. The new owner can now force to sell the asset even though it’s might not be relevant anymore.

• zAuction - Once a bid was accepted and the asset was transferred, all other bids need to be invalidated automatically, otherwise and old bid might be accepted even after the formal auction is over.

• zAuction, zNS - There is no way for the bidder to cancel an old bid. That might be useful in the event of a significant change in market trend, where the old pricing is no longer relevant. Currently, in order to cancel a bid, the bidder can either withdraw his ether balance from the zAuctionAccountant, or disapprove WETH which requires an extra transaction that might be front-runned by the seller.

Examples

zAuction/contracts/zAuction.sol:L35-L45

function acceptBid(bytes memory signature, uint256 rand, address bidder, uint256 bid, address nftaddress, uint256 tokenid) external {
    address recoveredbidder = recover(toEthSignedMessageHash(keccak256(abi.encode(rand, address(this), block.chainid, bid, nftaddress, tokenid))), signature);
    require(bidder == recoveredbidder, 'zAuction: incorrect bidder');
    require(!randUsed[rand], 'Random nonce already used');
    randUsed[rand] = true;
    IERC721 nftcontract = IERC721(nftaddress);
    accountant.Exchange(bidder, msg.sender, bid);
    nftcontract.transferFrom(msg.sender, bidder, tokenid);
    emit BidAccepted(bidder, msg.sender, bid, nftaddress, tokenid);
}

zNS/contracts/StakingController.sol:L120-L152

  function fulfillDomainBid(
  uint256 parentId,
  uint256 bidAmount,
  uint256 royaltyAmount,
  string memory bidIPFSHash,
  string memory name,
  string memory metadata,
  bytes memory signature,
  bool lockOnCreation,
  address recipient
) external {
  bytes32 recoveredBidHash = createBid(parentId, bidAmount, bidIPFSHash, name);
  address recoveredBidder = recover(recoveredBidHash, signature);
  require(recipient == recoveredBidder, "ZNS: bid info doesnt match/exist");
  bytes32 hashOfSig = keccak256(abi.encode(signature));
  require(approvedBids[hashOfSig] == true, "ZNS: has been fullfilled");
  infinity.safeTransferFrom(recoveredBidder, controller, bidAmount);
  uint256 id = registrar.registerDomain(parentId, name, controller, recoveredBidder);
  registrar.setDomainMetadataUri(id, metadata);
  registrar.setDomainRoyaltyAmount(id, royaltyAmount);
  registrar.transferFrom(controller, recoveredBidder, id);
  if (lockOnCreation) {
    registrar.lockDomainMetadataForOwner(id);
  }
  approvedBids[hashOfSig] = false;
  emit DomainBidFulfilled(
    metadata,
    name,
    recoveredBidder,
    id,
    parentId
  );
}

Recommendation

Consider adding an expiration field to the message signed by the bidder both for zAuction and zNS. Consider adding auction control, creating an auctionId, and have users bid on specific auctions. By adding this id to the signed message, all other bids are invalidated automatically and users would have to place new bids for a new auction. Optionally allow users to cancel bids explicitly.

Description

The zAuction initialization method is unprotected and while only being executable once, can be called by anyone. This might allow someone to monitor the mempool for new deployments of this contract and fron-run the initialization to initialize it with different parameters.

A mitigating factor is that this condition can be detected by the deployer as subsequent calls to init() will fail.

• Note: this doesn’t adhere to common interface naming convention/oz naming convention where this method would be called initialize.
• Note: that zNS in contrast relies on ou/initializable pattern with proper naming.
• Note: that this function might not be necessary at all and should be replaced by a constructor instead, as the contract is not used with a proxy pattern.

Examples

zAuction/contracts/zAuction.sol:L22-L26

function init(address accountantaddress) external {
    require(!initialized);
    initialized = true;
    accountant = zAuctionAccountant(accountantaddress);
}

Recommendation

The contract is not used in a proxy pattern, hence, the initialization should be performed in the constructor instead.

Description

zAuction appears to implement an upgrade path for the auction system via zAuctionAccountant. zAuction itself does not hold any value. The zAuctionAccountant can be configured to allow only one zAution contract to interact with it. The update of the contract reference takes effect immediately (https://github.com/ConsenSys/zer0-zauction-audit-2021-05/issues/7).

Acceptance of bids via the accountant on the old contract immediately fail after an admin updates the referenced zAuction contract while WETH bids may still continue. This may create an unfavorable scenario where two contracts may be active in parallel accepting WETH bids.

It should also be noted that 2nd layer bids (signed data) using the accountant for the old contract will not be acceptable anymore.

Examples

zAuction/contracts/zAuctionAccountant.sol:L60-L63

function SetZauction(address zauctionaddress) external onlyAdmin{
    zauction = zauctionaddress;
    emit ZauctionSet(zauctionaddress);
}

Recommendation

Consider re-thinking the upgrade path. Avoid keeping multiple versions of the auction contact active.

Description

The execution status of both zAuction.acceptBid and StakingController.fulfillDomainBid transactions depend on the bidder, as his approval is needed, his signature is being validated, etc. However, these transactions can be submitted by accounts that are different from the bidder account, or for accounts that do not have the required funds/deposits available, luring the account that has to perform the on-chain call into spending gas on a transaction that is deemed to fail (gas griefing). E.g. posting high-value fake bids for zAuction without having funds deposited or WETH approved.

Examples

zNS/contracts/StakingController.sol:L120-L152

  function fulfillDomainBid(
  uint256 parentId,
  uint256 bidAmount,
  uint256 royaltyAmount,
  string memory bidIPFSHash,
  string memory name,
  string memory metadata,
  bytes memory signature,
  bool lockOnCreation,
  address recipient
) external {
  bytes32 recoveredBidHash = createBid(parentId, bidAmount, bidIPFSHash, name);
  address recoveredBidder = recover(recoveredBidHash, signature);
  require(recipient == recoveredBidder, "ZNS: bid info doesnt match/exist");
  bytes32 hashOfSig = keccak256(abi.encode(signature));
  require(approvedBids[hashOfSig] == true, "ZNS: has been fullfilled");
  infinity.safeTransferFrom(recoveredBidder, controller, bidAmount);
  uint256 id = registrar.registerDomain(parentId, name, controller, recoveredBidder);
  registrar.setDomainMetadataUri(id, metadata);
  registrar.setDomainRoyaltyAmount(id, royaltyAmount);
  registrar.transferFrom(controller, recoveredBidder, id);
  if (lockOnCreation) {
    registrar.lockDomainMetadataForOwner(id);
  }
  approvedBids[hashOfSig] = false;
  emit DomainBidFulfilled(
    metadata,
    name,
    recoveredBidder,
    id,
    parentId
  );
}

zAuction/contracts/zAuction.sol:L35-L44

function acceptBid(bytes memory signature, uint256 rand, address bidder, uint256 bid, address nftaddress, uint256 tokenid) external {
    address recoveredbidder = recover(toEthSignedMessageHash(keccak256(abi.encode(rand, address(this), block.chainid, bid, nftaddress, tokenid))), signature);
    require(bidder == recoveredbidder, 'zAuction: incorrect bidder');
    require(!randUsed[rand], 'Random nonce already used');
    randUsed[rand] = true;
    IERC721 nftcontract = IERC721(nftaddress);
    accountant.Exchange(bidder, msg.sender, bid);
    nftcontract.transferFrom(msg.sender, bidder, tokenid);
    emit BidAccepted(bidder, msg.sender, bid, nftaddress, tokenid);
}

Recommendation

• Revert early for checks that depend on the bidder before performing gas-intensive computations.
• Consider adding a dry-run validation for off-chain components before transaction submission.

Description

The specification outlines three main user journeys of which one does not seem to be implemented.

1. Users will be able to do simple transfer of NFTs. - which does not require functionality in the smart contract
2. Users will be able to post NFTs at a sale price, and buy at that price. - does not seem to be implemented
3. Users will be able to post NFTs for auction, bid on auctions, and accept bids - is implemented

Recommendation

User flow (2) is not implemented in the smart contract system. Consider updating the spec or clearly highlighting functionality that is still in development for it to be excluded from security testing.

Description

zer0-os/zAuction@2f92aa1 introduced a way of tracking auctions/sales by using an auctionId/saleId. The id’s are unique and the same id cannot be used for multiple auctions/offers.

Two different auctions/offers may pick the same id, the first auction/offer will go through while the latter cannot be fulfilled anymore. This may happen accidentally or intentionally be forced by a malicious actor to terminate active auctions/sales (griefing, front-running).

Examples

Alice puts out an offer for someone to buy nft X at a specific price. Bob decides to accept that offer and buy the nft by calling zSale.purchase(saleid, price, token, ...). Mallory monitors the mempool, sees this transaction, front-runs it to fulfill its own sale (for a random nft he owns) reusing the saleid from Bobs transaction. Since Mallories transaction marks the saleid as consumed it terminates Alie’s offer and hence Bob cannot buy the token as the transaction will revert.

Recommendation

Consider using keccak(saleid+nftcontract+nfttokenid) as the unique sale/auction identifier instead, or alternatively associate the bidder address with the auctionId (require that consumed[bidder][auctionId]== false)

Description

The auction contract hardcodes the WETH ERC20 token address. this address will not be functional when deploying to mainnet.

Examples

zAuction/contracts/zAuction.sol:L15-L16

    IERC20 weth = IERC20(address(0xc778417E063141139Fce010982780140Aa0cD5Ab)); // rinkeby weth

Recommendation

Consider taking the used WETH token address as a constructor argument. Avoid code changes to facilitate testing! Perform input validation on arguments rejecting address(0x0) to facilitate the detection of potential misconfiguration in the deployment pipeline.

Description

Zero value transfers effectively perform a no-operation sometimes followed by calling out to the recipient of the withdrawal.

A transfer where from==to or where the value is 0 is ineffective.

Examples

zAuction/contracts/zAuctionAccountant.sol:L38-L42

function Withdraw(uint256 amount) external {
    ethbalance[msg.sender] = SafeMath.sub(ethbalance[msg.sender], amount);
    payable(msg.sender).transfer(amount);
    emit Withdrew(msg.sender, amount);
}

zAuction/contracts/zAuctionAccountant.sol:L33-L36

function Deposit() external payable {
    ethbalance[msg.sender] = SafeMath.add(ethbalance[msg.sender], msg.value);
    emit Deposited(msg.sender, msg.value);
}

zAuction/contracts/zAuctionAccountant.sol:L44-L58

    function zDeposit(address to) external payable onlyZauction {
        ethbalance[to] = SafeMath.add(ethbalance[to], msg.value);
        emit zDeposited(to, msg.value);
    }

    function zWithdraw(address from, uint256 amount) external onlyZauction {
        ethbalance[from] = SafeMath.sub(ethbalance[from], amount);
        emit zWithdrew(from, amount);
    }

    function Exchange(address from, address to, uint256 amount) external onlyZauction {
        ethbalance[from] = SafeMath.sub(ethbalance[from], amount);
        ethbalance[to] = SafeMath.add(ethbalance[to], amount);
        emit zExchanged(from, to, amount);
    }

Recommendation

Consider rejecting ineffective withdrawals (zero value) or at least avoid issuing a zero value ETH transfers. Avoid emitting successful events for ineffective calls to not trigger 3rd party components on noop’s.

Description

A seller can accept their own bid which is an ineffective action that is emitting an event.

Examples

zAuction/contracts/zAuction.sol:L35-L56

function acceptBid(bytes memory signature, uint256 rand, address bidder, uint256 bid, address nftaddress, uint256 tokenid) external {
    address recoveredbidder = recover(toEthSignedMessageHash(keccak256(abi.encode(rand, address(this), block.chainid, bid, nftaddress, tokenid))), signature);
    require(bidder == recoveredbidder, 'zAuction: incorrect bidder');
    require(!randUsed[rand], 'Random nonce already used');
    randUsed[rand] = true;
    IERC721 nftcontract = IERC721(nftaddress);
    accountant.Exchange(bidder, msg.sender, bid);
    nftcontract.transferFrom(msg.sender, bidder, tokenid);
    emit BidAccepted(bidder, msg.sender, bid, nftaddress, tokenid);
}

/// @dev 'true' in the hash here is the eth/weth switch
function acceptWethBid(bytes memory signature, uint256 rand, address bidder, uint256 bid, address nftaddress, uint256 tokenid) external {
    address recoveredbidder = recover(toEthSignedMessageHash(keccak256(abi.encode(rand, address(this), block.chainid, bid, nftaddress, tokenid, true))), signature);
    require(bidder == recoveredbidder, 'zAuction: incorrect bidder');
    require(!randUsed[rand], 'Random nonce already used');
    randUsed[rand] = true;
    IERC721 nftcontract = IERC721(nftaddress);
    weth.transferFrom(bidder, msg.sender, bid);
    nftcontract.transferFrom(msg.sender, bidder, tokenid);
    emit WethBidAccepted(bidder, msg.sender, bid, nftaddress, tokenid);
}

Recommendation

Disallow transfers to self.