Prepared by:
HALBORN
Last Updated 04/26/2024
Date of Engagement by: February 17th, 2022 - March 4th, 2022
90% of all REPORTED Findings have been addressed
All findings
10
Critical
0
High
0
Medium
1
Low
1
Informational
8
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.
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.
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)
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
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 analysis | Risk level | Remediation Date |
|---|---|---|
| OWNER IN ERC721DEFAULTAPPROVALMINIMAL.ISAPPROVEDOROWNER IS NOT CORRECTLY VERIFIED | Medium | Solved - 06/17/2022 |
| FUNCTION ERC721WOONKLYNFTREVEALWAVE.CHANGEHIDDENBASEURI MODIFIES THE WRONG STATE VARIABLE | Low | Solved - 03/22/2022 |
| REVEALWAVE.REVEALDATE IS NOT USED | Informational | Solved - 03/22/2022 |
| UNNEEDED INITIALIZATION OF UINT256 VARIABLES TO 0 | Informational | Solved - 03/22/2022 |
| USING ++I CONSUMES LESS GAS THAN I++ IN LOOPS | Informational | Solved - 03/22/2022 |
| POSSIBLE MISUSE OF PUBLIC FUNCTIONS | Informational | Acknowledged |
| WOONKLYMINTING.MINT FUNCTION DOES MULTIPLE ETHER/TOKEN TRANSFER | Informational | - |
| WOONKLYMINTING.SAFETRANSFERFROM PRIVATE FUNCTION CAN BE REMOVED | Informational | Solved - 08/22/2022 |
| MISSING FEE LIMIT DEFINITION | Informational | Solved - 08/22/2022 |
| MISSING STATE VARIABLE VISIBILITY | Informational | Acknowledged |
// Medium
The contract ERC721DefaultApprovalMinimal contains the function isApprovedOrOwner():
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.
//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
SOLVED: The Woonkly team modified the condition parameters to correctly check for ownerOf(tokenId) instead of msg.sender() and spender.
// Low
The contracts ERC721WoonklyNFTRevealWave, ERC1155WoonklyNFTRevealWave and ERC721WoonklyNFTMinimalRevealWave contain the function changeHiddenBaseURI():
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.
SOLVED: The Woonkly team modified the changeHiddenBaseURI() function as suggested.
// Informational
The contracts ERC721WoonklyNFTRevealWave, ERC1155WoonklyNFTRevealWave and `ERC721WoonklyNFTMinimalRevealWave`` contain the following struct:
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:
function changeRevealDate(
uint _waveId,
uint _revealDate
) public onlyOwner {
RevealWave storage revealWave = waves[_waveId];
revealWave.revealDate = _revealDate;
}
SOLVED: The Woonkly team removed the revealDate variable from the RevealWave struct. The changeRevealDate() setter function was also removed.
// Informational
As i is an uint256, it is already initialized to 0. uint256 i = 0 reassigns the 0 to i which wastes gas.
ERC721LazyMinimal.sol
for (uint i = 0; i < _creators.length; i++) {WoonklyNFTTransferManager.sol
for (uint256 i = 0; i < payouts.length - 1; i++) {for (uint256 i = 0; i < orderOriginFees.length; i++) {ERC1155Lazy.sol
for (uint i = 0; i < data.creators.length; i++) {for (uint i = 0; i < _creators.length; i++) {ERC1155Base.sol
for (uint i = 0; i < ids.length; i++) {ERC1155Upgradeable.sol
for (uint256 i = 0; i < accounts.length; ++i) {for (uint256 i = 0; i < ids.length; ++i) {for (uint i = 0; i < ids.length; i++) {for (uint i = 0; i < ids.length; i++) {RoyaltiesRegistry.sol
for (uint i = 0; i < royalties.length; i++) {for (uint256 i = 0; i < values.length; i++) {ERC721Lazy.sol
for (uint i = 0; i < data.creators.length; i++) {for (uint i = 0; i < _creators.length; i++) {ERC721WoonklyNFTRevealWave.sol
for (uint256 i = 0; i < operators.length; i++) {SOLVED: The Woonkly team removed the initialization of the uint256 variable to 0 in the for loops mentioned reducing the gas costs.
// Informational
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++.
ERC721LazyMinimal.sol
for (uint i = 0; i < _creators.length; i++) {WoonklyNFTTransferManager.sol
for (uint256 i = 0; i < payouts.length - 1; i++) {for (uint256 i = 0; i < orderOriginFees.length; i++) {ERC1155Lazy.sol
for (uint i = 0; i < data.creators.length; i++) {for (uint i = 0; i < _creators.length; i++) {ERC1155Base.sol
for (uint i = 0; i < ids.length; i++) {ERC1155Upgradeable.sol
for (uint i = 0; i < ids.length; i++) {for (uint i = 0; i < ids.length; i++) {RoyaltiesRegistry.sol
for (uint i = 0; i < royalties.length; i++) {for (uint256 i = 0; i < values.length; i++) {ERC721Lazy.sol
for (uint i = 0; i < data.creators.length; i++) {for (uint i = 0; i < _creators.length; i++) {ERC721WoonklyNFTRevealWave.sol
for (uint256 i = 0; i < operators.length; i++) {For example, based in the following test contract:
//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:
SOLVED: The Woonkly team now uses ++i instead of i++ to increment the iterator variable in for loops reducing the gas costs.
// Informational
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)ACKNOWLEDGED: The Woonkly team acknowledged this issue.
// Informational
// Informational
In the WoonklyMinting contract, the function _safeTransferFrom() is a private function that just does a external safeTransferFrom() call:
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.
SOLVED: The Woonkly team now uses the SafeERC20.safeTransferFrom() function.
// Informational
During the tests, Halborn Team noticed that on the setFees function, limits are not defined and could be set with values greater than 100%.
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);
}
SOLVED: The Woonkly team added fee limitation to avoid fees higher than 100%.
// Informational
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.
contract OperatorRole2 is OwnableUpgradeable2 {
mapping (address => bool) operators;
ACKNOWLEDGED: The Woonkly team acknowledged this finding.
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.
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.
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.
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.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed