Halborn Logo

NFT protocol - Woonkly


Prepared by:

Halborn Logo

HALBORN

Last Updated 04/26/2024

Date of Engagement by: February 17th, 2022 - March 4th, 2022

Summary

90% of all REPORTED Findings have been addressed

All findings

10

Critical

0

High

0

Medium

1

Low

1

Informational

8


1. INTRODUCTION

Woonkly engaged Halborn to conduct a security audit on their staking smart contracts beginning on February 17th, 2022 and ending on March 4th, 2022. The security assessment was scoped to the smart contracts provided in the GitHub repository Woonkly/nft-protocol-contracts.

2. AUDIT SUMMARY

The team at Halborn was provided three weeks for the engagement and assigned a full-time security engineer to audit the security of the smart contract. The security engineer is a blockchain and smart-contract security expert with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.

The purpose of this audit is to:

    • Ensure that smart contract functions operate as intended

    • Identify potential security issues within the smart contracts

In summary, Halborn identified some security risks that were mostly addressed by Woonkly team.

3. TEST APPROACH & METHODOLOGY

Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this audit. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the code and can quickly identify items that do not follow the security best practices. The following phases and associated tools were used during the audit:

    • Research into architecture and purpose

    • Smart contract manual code review and walkthrough

    • Graphing out functionality and contract logic/connectivity/functions (solgraph)

    • Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic related vulnerability classes

    • Manual testing by custom scripts

    • Scanning of solidity files for vulnerabilities, security hotspots or bugs. (MythX)

    • Static Analysis of security for scoped contract, and imported functions. (Slither)

    • Testnet deployment (Brownie, Remix IDE)

4. SCOPE

IN-SCOPE: The security assessment was scoped to the following smart contracts:

    • ProxyAdmin.sol

    • ExchangeV2.sol

    • RoyaltiesRegistry.sol

    • ERC721WoonklyNFT.sol

    • ERC721WoonklyNFTMinimal.sol

    • ERC721WoonklyNFTRevealWave.sol

    • ERC721WoonklyNFTMinimalRevealWave.sol

    • ERC721WoonklyNFTFactoryC2.sol

    • ERC721WoonklyNFTRevealWaveFactoryC2.sol

    • ERC721WoonklyNFTMinimalBeacon.sol

    • ERC721WoonklyNFTBeacon.sol

    • ERC1155WoonklyNFT.sol

    • ERC1155WoonklyNFTRevealWave.sol

    • ERC1155WoonklyNFTFactoryC2.sol

    • ERC1155WoonklyNFTRevealWaveFactoryC2.sol

    • ERC1155WoonklyNFTBeacon.sol

    • ERC721LazyMintTransferProxy.sol

    • ERC1155LazyMintTransferProxy.sol

    • TransferProxy.sol

    • ERC20TransferProxy.sol

    • AssetMatcherCollection.sol

    • WoonklyMintingFactory.sol

    • WoonklyMinting.sol

1st Commit ID: 5418fe74eafa63d8211b1689031ba2c4652af285 2nd Commit ID: 494370ffa011530f1db0a84993a0f62eee77da4e 3rd Commit ID: b32c8d296eab1ae401a055209427c62c399d5257 4th Commit ID: f33b4f50c7528eb6fc9fa673a711b08884798504 5th Commit ID: 0ad72fe5b64774265eee102b4c31fc1eed75eea6 6th Commit ID: ba4b92ee2d019dc8197c6d016668e127fa6b675 7th Commit ID: ba4b92ee2d019dc8197c6d016668e127fa6b675 8th Commit ID: 22880f6e425fdb44fca4b36013a3dc234545ba71

5. RISK METHODOLOGY

Vulnerabilities or issues observed by Halborn are ranked based on the risk assessment methodology by measuring the LIKELIHOOD of a security incident and the IMPACT should an incident occur. This framework works for communicating the characteristics and impacts of technology vulnerabilities. The quantitative model ensures repeatable and accurate measurement while enabling users to see the underlying vulnerability characteristics that were used to generate the Risk scores. For every vulnerability, a risk level will be calculated on a scale of 5 to 1 with 5 being the highest likelihood or impact.
RISK SCALE - LIKELIHOOD
  • 5 - Almost certain an incident will occur.
  • 4 - High probability of an incident occurring.
  • 3 - Potential of a security incident in the long term.
  • 2 - Low probability of an incident occurring.
  • 1 - Very unlikely issue will cause an incident.
RISK SCALE - IMPACT
  • 5 - May cause devastating and unrecoverable impact or loss.
  • 4 - May cause a significant level of impact or loss.
  • 3 - May cause a partial impact or loss to many.
  • 2 - May cause temporary impact or loss.
  • 1 - May cause minimal or un-noticeable impact.
The risk level is then calculated using a sum of these two values, creating a value of 10 to 1 with 10 being the highest level of security risk.
Critical
High
Medium
Low
Informational
  • 10 - CRITICAL
  • 9 - 8 - HIGH
  • 7 - 6 - MEDIUM
  • 5 - 4 - LOW
  • 3 - 1 - VERY LOW AND INFORMATIONAL

6. SCOPE

Out-of-Scope: New features/implementations after the remediation commit IDs.

7. Assessment Summary & Findings Overview

Critical

0

High

0

Medium

1

Low

1

Informational

8

Impact x Likelihood

HAL-02

HAL-01

HAL-03

HAL-04

HAL-05

HAL-06

HAL-07

HAL-08

HAL-09

HAL-10

Security analysisRisk levelRemediation Date
OWNER IN ERC721DEFAULTAPPROVALMINIMAL.ISAPPROVEDOROWNER IS NOT CORRECTLY VERIFIEDMediumSolved - 06/17/2022
FUNCTION ERC721WOONKLYNFTREVEALWAVE.CHANGEHIDDENBASEURI MODIFIES THE WRONG STATE VARIABLELowSolved - 03/22/2022
REVEALWAVE.REVEALDATE IS NOT USEDInformationalSolved - 03/22/2022
UNNEEDED INITIALIZATION OF UINT256 VARIABLES TO 0InformationalSolved - 03/22/2022
USING ++I CONSUMES LESS GAS THAN I++ IN LOOPSInformationalSolved - 03/22/2022
POSSIBLE MISUSE OF PUBLIC FUNCTIONSInformationalAcknowledged
WOONKLYMINTING.MINT FUNCTION DOES MULTIPLE ETHER/TOKEN TRANSFERInformational-
WOONKLYMINTING.SAFETRANSFERFROM PRIVATE FUNCTION CAN BE REMOVEDInformationalSolved - 08/22/2022
MISSING FEE LIMIT DEFINITIONInformationalSolved - 08/22/2022
MISSING STATE VARIABLE VISIBILITYInformationalAcknowledged

8. Findings & Tech Details

8.1 OWNER IN ERC721DEFAULTAPPROVALMINIMAL.ISAPPROVEDOROWNER IS NOT CORRECTLY VERIFIED

// Medium

Description

The contract ERC721DefaultApprovalMinimal contains the function isApprovedOrOwner():

ERC721WoonklyNFTRevealWave.sol

function _isApprovedOrOwner(address spender, uint256 tokenId) internal virtual override view returns (bool) {
        return !rejectedDefaultApprovals[_msgSender()][spender] && (spender==transferProxy || spender==extraOperator || super._isApprovedOrOwner(spender, tokenId));
    }

This function returns if an address is approved or the owner for transactions but the logic followed to determine if he is approved or is the owner is not correct. It is possible to get a true result using an address that is not actually approved nor owner.

test.js

    //minting 5 tokens to owner
    await token.mint(5, 0, [], { from: owner });

    let res = await token.isApprovedOrOwner(owner, 1, { from: owner });
    console.log("1 - owner approved owner, it should be true. The result is " + res);

    res = await token.isApprovedOrOwner(owner, 1, { from: accounts[5] });
    console.log("2 - random addr approved owner, it should be false. The result is " + res);

When the above tests are executed, the following result is obtained.

1 - owner approved owner, it should be true. The result is true
2 - random addr approved owner, it should be false. The result is true
Score
Impact: 3
Likelihood: 3
Recommendation

SOLVED: The Woonkly team modified the condition parameters to correctly check for ownerOf(tokenId) instead of msg.sender() and spender.

8.2 FUNCTION ERC721WOONKLYNFTREVEALWAVE.CHANGEHIDDENBASEURI MODIFIES THE WRONG STATE VARIABLE

// Low

Description

The contracts ERC721WoonklyNFTRevealWave, ERC1155WoonklyNFTRevealWave and ERC721WoonklyNFTMinimalRevealWave contain the function changeHiddenBaseURI():

ERC721WoonklyNFTRevealWave.sol

function changeHiddenBaseURI(
    uint _waveId,
    string memory _baseURI
) public onlyOwner {
    require(bytes(_baseURI).length > 0,"Error: Input parameters can not be empty (string)");
    RevealWave storage revealWave = waves[_waveId];
    revealWave.revealBaseURI = _baseURI;
}

As we can see in the code above, the function is modifying the revealBaseURI variable instead of the hiddenBaseURI. As the name of the function indicates, this is not correct.

Score
Impact: 3
Likelihood: 2
Recommendation

SOLVED: The Woonkly team modified the changeHiddenBaseURI() function as suggested.

8.3 REVEALWAVE.REVEALDATE IS NOT USED

// Informational

Description

The contracts ERC721WoonklyNFTRevealWave, ERC1155WoonklyNFTRevealWave and `ERC721WoonklyNFTMinimalRevealWave`` contain the following struct:

ERC721WoonklyNFTRevealWave.sol

    struct RevealWave {
        bool isRevealed;
        string name;
        string hiddenBaseURI;
        bool addTokenURIToHiddenBaseURI;
        string revealBaseURI;
        uint revealDate;
    }

The revealDate parameter is not used anywhere in the code and provides no utility, for this reason, it can be removed from the struct. There is only a setter function that can also be removed:

ERC721WoonklyNFTRevealWave.sol

function changeRevealDate(
    uint _waveId,
    uint _revealDate
) public onlyOwner {
    RevealWave storage revealWave = waves[_waveId];
    revealWave.revealDate = _revealDate;   
}
Score
Impact: 1
Likelihood: 1
Recommendation

SOLVED: The Woonkly team removed the revealDate variable from the RevealWave struct. The changeRevealDate() setter function was also removed.

8.4 UNNEEDED INITIALIZATION OF UINT256 VARIABLES TO 0

// Informational

Description

As i is an uint256, it is already initialized to 0. uint256 i = 0 reassigns the 0 to i which wastes gas.

Code Location

ERC721LazyMinimal.sol

  • Line 59: `for (uint i = 0; i < data.creators.length; i++) {``
  • Line 85: for (uint i = 0; i < _creators.length; i++) {

WoonklyNFTTransferManager.sol

  • Line 165: `for (uint256 i = 0; i < fees.length; i++) {``
  • Line 184: for (uint256 i = 0; i < payouts.length - 1; i++) {
  • Line 206: for (uint256 i = 0; i < orderOriginFees.length; i++) {

ERC1155Lazy.sol

  • Line 70: for (uint i = 0; i < data.creators.length; i++) {
  • Line 117: for (uint i = 0; i < _creators.length; i++) {

ERC1155Base.sol

  • Line 31: for (uint i = 0; i < ids.length; i++) {

ERC1155Upgradeable.sol

  • Line 119: for (uint256 i = 0; i < accounts.length; ++i) {
  • Line 200: for (uint256 i = 0; i < ids.length; ++i) {
  • Line 280: for (uint i = 0; i < ids.length; i++) {
  • Line 327: for (uint i = 0; i < ids.length; i++) {

RoyaltiesRegistry.sol

  • Line 99: for (uint i = 0; i < royalties.length; i++) {
  • Line 225: for (uint256 i = 0; i < values.length; i++) {

ERC721Lazy.sol

  • Line 63: for (uint i = 0; i < data.creators.length; i++) {
  • Line 99: for (uint i = 0; i < _creators.length; i++) {

ERC721WoonklyNFTRevealWave.sol

  • Line 47: for (uint256 i = 0; i < operators.length; i++) {
Score
Impact: 1
Likelihood: 1
Recommendation

SOLVED: The Woonkly team removed the initialization of the uint256 variable to 0 in the for loops mentioned reducing the gas costs.

8.5 USING ++I CONSUMES LESS GAS THAN I++ IN LOOPS

// Informational

Description

In the loop below, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++.

Code Location

ERC721LazyMinimal.sol

  • Line 59: `for (uint i = 0; i < data.creators.length; i++) {``
  • Line 85: for (uint i = 0; i < _creators.length; i++) {

WoonklyNFTTransferManager.sol

  • Line 165: `for (uint256 i = 0; i < fees.length; i++) {``
  • Line 184: for (uint256 i = 0; i < payouts.length - 1; i++) {
  • Line 206: for (uint256 i = 0; i < orderOriginFees.length; i++) {

ERC1155Lazy.sol

  • Line 70: for (uint i = 0; i < data.creators.length; i++) {
  • Line 117: for (uint i = 0; i < _creators.length; i++) {

ERC1155Base.sol

  • Line 31: for (uint i = 0; i < ids.length; i++) {

ERC1155Upgradeable.sol

  • Line 280: for (uint i = 0; i < ids.length; i++) {
  • Line 327: for (uint i = 0; i < ids.length; i++) {

RoyaltiesRegistry.sol

  • Line 99: for (uint i = 0; i < royalties.length; i++) {
  • Line 225: for (uint256 i = 0; i < values.length; i++) {

ERC721Lazy.sol

  • Line 63: for (uint i = 0; i < data.creators.length; i++) {
  • Line 99: for (uint i = 0; i < _creators.length; i++) {

ERC721WoonklyNFTRevealWave.sol

  • Line 47: for (uint256 i = 0; i < operators.length; i++) {

For example, based in the following test contract:

Test.sol

//SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i++) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

We can see the difference in the gas costs: posti_prei.png

Score
Impact: 1
Likelihood: 1
Recommendation

SOLVED: The Woonkly team now uses ++i instead of i++ to increment the iterator variable in for loops reducing the gas costs.

8.6 POSSIBLE MISUSE OF PUBLIC FUNCTIONS

// Informational

Description

In the contracts below contracts, there are some functions marked as public that are never called directly within the contract itself or in any of their descendants:

ProxyAdmin.sol

  • getProxyImplementation() (ProxyAdmin.sol#19-31)
  • getProxyAdmin() (ProxyAdmin.sol#37-49)
  • changeProxyAdmin() (ProxyAdmin.sol#56-61)
  • upgrade() (ProxyAdmin.sol#68-73)
  • upgradeAndCall() (ProxyAdmin.sol#84-90)

ERC721WoonklyNFTRevealWave.sol

  • setRevealWave() (ERC721WoonklyNFTRevealWave.sol#104-117)
  • changeHiddenBaseURI() (ERC721WoonklyNFTRevealWave.sol#119-126)
  • changeAddTokenURIToHiddenBaseURI() (ERC721WoonklyNFTRevealWave.sol#128-134)
  • changeRevealBaseURI() (ERC721WoonklyNFTRevealWave.sol#136-145)
  • changeRevealDate() (ERC721WoonklyNFTRevealWave.sol#147-153)
  • resetIsRevealedAndRevealURI() (ERC721WoonklyNFTRevealWave.sol#155-161)
  • changeName() (ERC721WoonklyNFTRevealWave.sol#164-171)
  • mintAndTransferReveal() (ERC721WoonklyNFTRevealWave.sol#233-243)

ERC721WoonklyNFTMinimalRevealWave.sol

  • setRevealWave() (ERC721WoonklyNFTMinimalRevealWave.sol#70-83)
  • changeHiddenBaseURI() (ERC721WoonklyNFTMinimalRevealWave.sol#85-92)
  • changeAddTokenURIToHiddenBaseURI() (ERC721WoonklyNFTMinimalRevealWave.sol#94-100)
  • changeRevealBaseURI() (ERC721WoonklyNFTMinimalRevealWave.sol#102-111)
  • changeRevealDate() (ERC721WoonklyNFTMinimalRevealWave.sol#113-119)
  • resetIsRevealedAndRevealURI() (ERC721WoonklyNFTMinimalRevealWave.sol#121-127)
  • changeName() (ERC721WoonklyNFTMinimalRevealWave.sol#130-137)
  • mintAndTransferReveal() (ERC721WoonklyNFTMinimalRevealWave.sol#199-209)

ERC721WoonklyNFTFactoryC2.sol

  • getAddress() (ERC721WoonklyNFTFactoryC2.sol#61-73)
  • getAddress() (ERC721WoonklyNFTFactoryC2.sol#80-92)

ERC721WoonklyNFTRevealWaveFactoryC2.sol

  • getAddress() (ERC721WoonklyNFTRevealWaveFactoryC2.sol#64-76)
  • getAddress() (ERC721WoonklyNFTRevealWaveFactoryC2.sol#83-95)

ERC1155WoonklyNFTRevealWave.sol

  • setRevealWave() (ERC1155WoonklyNFTRevealWave.sol#72-85)
  • changeHiddenBaseURI() (ERC1155WoonklyNFTRevealWave.sol#87-94)
  • changeAddTokenURIToHiddenBaseURI() (ERC1155WoonklyNFTRevealWave.sol#96-102)
  • changeRevealBaseURI() (ERC1155WoonklyNFTRevealWave.sol#104-113)
  • changeRevealDate() (ERC1155WoonklyNFTRevealWave.sol#115-121)
  • resetIsRevealedAndRevealURI() (ERC1155WoonklyNFTRevealWave.sol#123-129)
  • changeName() (ERC1155WoonklyNFTRevealWave.sol#132-139)
  • assignRevealWaveIdToTokenId() (ERC1155WoonklyNFTRevealWave.sol#142-145)

ERC1155WoonklyNFTFactoryC2.sol

  • getAddress() (ERC1155WoonklyNFTFactoryC2.sol#63-75)
  • getAddress() (ERC1155WoonklyNFTFactoryC2.sol#82-94)

ERC1155WoonklyNFTRevealWaveFactoryC2.sol

  • getAddress() (ERC1155WoonklyNFTRevealWaveFactoryC2.sol#63-75)
  • getAddress() (ERC1155WoonklyNFTRevealWaveFactoryC2.sol#82-94)
Score
Impact: 1
Likelihood: 1
Recommendation

ACKNOWLEDGED: The Woonkly team acknowledged this issue.

8.7 WOONKLYMINTING.MINT FUNCTION DOES MULTIPLE ETHER/TOKEN TRANSFER

// Informational

Description
Finding description placeholder
Score
Impact:
Likelihood:

8.8 WOONKLYMINTING.SAFETRANSFERFROM PRIVATE FUNCTION CAN BE REMOVED

// Informational

Description

In the WoonklyMinting contract, the function _safeTransferFrom() is a private function that just does a external safeTransferFrom() call:

WoonklyMinting.sol

function _safeTransferFrom(
    IERC20 _token,
    address _sender,
    address _recipient,
    uint256 _amount
) private {
    SafeERC20.safeTransferFrom(_token, _sender, _recipient, _amount);
}

\color{black} \color{white}

The declaration of this function does not provide any utility and increased the deployment gas costs. For that reason, it is recommended to remove it and just use in the smart contract, when needed, the SafeERC20.safeTransferFrom(_token, _sender, _recipient, _amount); call.

Score
Impact: 1
Likelihood: 1
Recommendation

SOLVED: The Woonkly team now uses the SafeERC20.safeTransferFrom() function.

8.9 MISSING FEE LIMIT DEFINITION

// Informational

Description

During the tests, Halborn Team noticed that on the setFees function, limits are not defined and could be set with values greater than 100%.

WoonklyMinting.sol

    function setFees(address _erc20Token, uint256 _fee) external onlyOwner {

        FeesData storage feesData = erc20TokenToFeesData[_erc20Token];
        require(feesData.fee != _fee, "same fee");
        feesData.fee = _fee;
        emit SetERC20Token(_erc20Token, _fee, feesData.feesReceiver, feesData.isTradeable);
    }
Score
Impact: 1
Likelihood: 1
Recommendation

SOLVED: The Woonkly team added fee limitation to avoid fees higher than 100%.

8.10 MISSING STATE VARIABLE VISIBILITY

// Informational

Description

During the tests, Halborn Team noticed that the mapping operators declared as state variable in the ERC20TransferProxyFlatAndUpdated.sol contract does not have any visibility defined. The Ethereum network set this visibility as internal by default. This may cause execution problems if this operators mapping is intended to be used as a public state variable.

ERC20TransferProxyFlatAndUpdated.sol

    contract OperatorRole2 is OwnableUpgradeable2 {
    mapping (address => bool) operators;
Score
Impact: 1
Likelihood: 1
Recommendation

ACKNOWLEDGED: The Woonkly team acknowledged this finding.

9. Automated Testing

STATIC ANALYSIS REPORT

Description

Halborn used automated testing techniques to enhance the coverage of certain areas of the smart contracts in scope. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified the smart contracts in the repository and was able to compile them correctly into their abis and binary format, Slither was run against the contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.

Slither results

ProxyAdmin.sol

ExchangeV2.sol

RoyaltiesRegistry.sol

ERC721WoonklyNFT.sol

ERC721WoonklyNFTMinimal.sol

ERC721WoonklyNFTRevealWave.sol

ERC721WoonklyNFTMinimalRevealWave.sol

ERC721WoonklyNFTFactoryC2.sol

ERC721WoonklyNFTRevealWaveFactoryC2.sol

ERC721WoonklyNFTMinimalBeacon.sol

ERC721WoonklyNFTBeacon.sol

ERC1155WoonklyNFT.sol

ERC1155WoonklyNFTRevealWave.sol

ERC1155WoonklyNFTFactoryC2.sol

ERC1155WoonklyNFTRevealWaveFactoryC2.sol

ERC1155WoonklyNFTBeacon.sol

ERC721LazyMintTransferProxy.sol

ERC1155LazyMintTransferProxy.sol

TransferProxy.sol

ERC20TransferProxy.sol

AssetMatcherCollection.sol

WoonklyMintingFactory.sol

WoonklyMinting.sol

  • No major issues found by Slither.

AUTOMATED SECURITY SCAN

Description

Halborn used automated security scanners to assist with detection of well-known security issues and to identify low-hanging fruits on the targets for this engagement. Among the tools used was MythX, a security analysis service for Ethereum smart contracts. MythX performed a scan on the smart contracts and sent the compiled results to the analyzers in order to locate any vulnerabilities.

MythX results

ProxyAdmin.sol No issues found by MythX

ExchangeV2.sol No issues found by MythX

RoyaltiesRegistry.sol

ERC721WoonklyNFT.sol No issues found by MythX

ERC721WoonklyNFTMinimal.sol

ERC721WoonklyNFTRevealWave.sol

ERC721WoonklyNFTMinimalRevealWave.sol

ERC721WoonklyNFTFactoryC2.sol

ERC721WoonklyNFTRevealWaveFactoryC2.sol

ERC721WoonklyNFTMinimalBeacon.sol

ERC721WoonklyNFTBeacon.sol

ERC1155WoonklyNFT.sol

ERC1155WoonklyNFTRevealWave.sol

ERC1155WoonklyNFTFactoryC2.sol

ERC1155WoonklyNFTRevealWaveFactoryC2.sol

ERC1155WoonklyNFTBeacon.sol

ERC721LazyMintTransferProxy.sol

ERC1155LazyMintTransferProxy.sol

TransferProxy.sol

ERC20TransferProxy.sol

AssetMatcherCollection.sol No issues found by MythX

WoonklyMintingFactory.sol

WoonklyMinting.sol

  • The Integer Overflows and Underflows flagged by MythX were checked individually and were determined to be mathematically impossible.

  • Assert violations are false positives.

Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.

© Halborn 2024. All rights reserved.