Prepared by:
HALBORN
Last Updated 04/26/2024
Date of Engagement by: April 11th, 2022 - April 13th, 2022
Summary
100% of all REPORTED Findings have been addressed
All findings
4
Critical
0
High
0
Medium
0
Low
1
Informational
3
Table of Contents
1. INTRODUCTION
\client engaged Halborn to conduct a security audit on their smart contracts beginning on 2022-04-11 and ending on April 13th, 2022. The security assessment was scoped to the smart contracts provided to the Halborn team.
It should be noted that the assessment was timeboxed and only three days were provided to assess the security of the smart contracts before public release.
The smart contracts within the scope provided the functionality of swapping Seascape's NFTs via their web platform. Users are able to initiate a swap request by offering up to five NFTs plus an optional bounty, and request up to five NFTs. Users of the platform with the requested NFTs can accept the swap by transferring their NFTs in exchange for the offered ones plus the optional bounty.
2. AUDIT SUMMARY
The team at Halborn was provided three days 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 with the smart contracts
In summary, Halborn identified some security risks that were mostly addressed by the \client team.
3. TEST APPROACH & METHODOLOGY
Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy regarding the scope of the smart contract audit. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of smart contracts and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the audit:
Research into architecture and purpose.
Smart Contract manual code review and walkthrough.
Dynamic Analysis (
ganache-cli,brownie,hardhat).
4. SCOPE
The assessment was scoped to the public GitHub repository on the \href{https://github.com/blocklords/seascape-smartcontracts/tree/nft-marketplace}{nft-marketplace branch}. The smart contracts in scope were inside the contracts/nft_swap directory.
The audited commit was 14a4eb1969eefa582a63dc3805186be08a38972f and fixes were assessed on newer commits.
5. RISK METHODOLOGY
- 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.
- 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.
- 10 - CRITICAL
- 9 - 8 - HIGH
- 7 - 6 - MEDIUM
- 5 - 4 - LOW
- 3 - 1 - VERY LOW AND INFORMATIONAL
6. SCOPE
7. Assessment Summary & Findings Overview
Critical
0
High
0
Medium
0
Low
1
Informational
3
Impact x Likelihood
HAL-01
HAL-02
HAL-03
HAL-04
| Security analysis | Risk level | Remediation Date |
|---|---|---|
| MISSING REENTRANCY GUARD | Low | Solved - 04/12/2022 |
| NFT NUMBER 0 CAN NEVER BE TRADED | Informational | Solved - 04/12/2022 |
| UPGRADE TO AT LEAST PRAGMA 0.8.10 | Informational | Acknowledged |
| USING ++I CONSUMES LESS GAS THAN I++ IN LOOPS | Informational | Solved - 04/12/2022 |
8. Findings & Tech Details
8.1 MISSING REENTRANCY GUARD
// Low
Description
During the audit, it was discovered that the cancelOffer function did not implement protections against reentrancy attacks. To protect against reentrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdrawal function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that protects the function against re-entrancy attacks.
Halborn attempted to exploit a possible reentrancy vulnerability in the cancelOffer function. However, it was not possible to successfully exploit the issue due to the NFT transfer not being successful after deletion of the offer index on the second iteration of the re-entered function.
Code Location
The affected function was cancelOffer on line 396.
Score
Likelihood: 2
Recommendation
SOLVED: The Seascape team added the nonReentrant modifier.
\newpage
8.2 NFT NUMBER 0 CAN NEVER BE TRADED
// Informational
Description
While currently supported NFTs start with tokenId equal to one, should additional NFT series be supported, with the first tokenId being zero, they would not be able to be traded.
Code Location
Location: NftSwap.sol - function createOffer
require(_offeredTokens[index].tokenId > 0, "nft id must be greater than 0");
Location: NftSwap.sol - function acceptOffer
require(_requestedTokenIds[i] > 0, "nft id must be greater than 0");
Score
Likelihood: 1
Recommendation
SOLVED: The Seascape team removed the require statements.
\newpage
8.3 UPGRADE TO AT LEAST PRAGMA 0.8.10
// Informational
Description
Gas optimizations and additional safety checks are available for free when using newer compiler versions and the optimizer.
- Safemath by default from 0.8.0 (can be more gas efficient than the SafeMath library)
- Low level inliner: from 0.8.2, leads to cheaper runtime gas. This is especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
- Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
- Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.
Code Location
The contracts within scope made use of the pragma version 0.6.7
Score
Likelihood: 1
Recommendation
ACKNOWLEDGED: The Seascape team decided not to upgrade to the latest pragma versions and instead deploy with the current one. They confirmed that future projects will use the newest versions.
\newpage
8.4 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++. This does not only apply to the iterator variable. It also applies to variables declared within the loop code block.
Code Location
NFTSwap.sol
- Line 236
- Line 246
- Line 260
- Line 287
- Line 290
- Line 333
- Line 355
- Line 360
- Line 402
Score
Likelihood: 1
Recommendation
SOLVED: The Seascape team now uses ++i to increment variables inside loops, saving some gas.
\newpage
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.
Table of Contents
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed