Boxed Static Analysis Findings

BDE-01S: Illegible Numeric Value Representations

Type	Severity	Location
Code Style		Boxed.sol: • I-1: L35 • I-2: L36 • I-3: L200 • I-4: L201

Description:

The linked representations of numeric literals are sub-optimally represented decreasing the legibility of the codebase.

Example:

Copy
35uint256 public constant MAX_SUPPLY = 10000;

Recommendation:

To properly illustrate each value's purpose, we advise the following guidelines to be followed. For values meant to depict fractions with a base of 1e18, we advise fractions to be utilized directly (i.e. 1e17 becomes 0.1e18) as they are supported. For values meant to represent a percentage base, we advise each value to utilize the underscore (_) separator to discern the percentage decimal (i.e. 10000 becomes 100_00, 300 becomes 3_00 and so on). Finally, for large numeric values we simply advise the underscore character to be utilized again to represent them (i.e. 1000000 becomes 1_000_000).

Alleviation:

The referenced value literals have been updated in their representation to 10_000, 100_00, 100_000, and 50_000 respectively in accordance with the recommendation's underscore style, addressing this exhibit.

BDE-02S: Inexistent Event Emissions

Type	Severity	Location
Language Specific		Boxed.sol: • I-1: L168-L172 • I-2: L175-L180 • I-3: L183-L188 • I-4: L190-L194

Description:

The linked functions adjust sensitive contract variables yet do not emit an event for it.

Example:

Copy
168function setTreasury(address _treasury) external onlyRole(DEFAULT_ADMIN_ROLE) {
169    require(_treasury != address(0), "Invalid treasury address");
170
171    treasury = _treasury;
172}

Recommendation:

We advise an event to be declared and correspondingly emitted for each function to ensure off-chain processes can properly react to this system adjustment.

Alleviation:

The TreasurySet, TokensPerBoxSet, FeeBPSSet, and StartTimestampSet events were introduced to the codebase and are correspondingly emitted in the Boxed::setTreasury, Boxed::setTokensPerBox, Boxed::setFeeBPS, and Boxed::setStartTimestamp functions respectively, addressing this exhibit in full.

BDE-03S: Inexistent Sanitization of Input Addresses

Type	Severity	Location
Input Sanitization		Boxed.sol:L51-L79

Description:

The linked function(s) accept address arguments yet do not properly sanitize them.

Impact:

The presence of zero-value addresses, especially in constructor implementations, can cause the contract to be permanently inoperable. These checks are advised as zero-value inputs are a common side-effect of off-chain software related bugs.

Example:

Copy
51constructor(
52    string memory _name,
53    string memory _symbol,
54    string memory _standardBaseURI,
55    string memory _goldBaseURI,
56    uint256 _tokensPerBox,
57    uint256 _feeBPS,
58    uint256 _startTimestamp,
59    address _meme,
60    address _unboxed,
61    address _treasury,
62    address _vrfWrapper
63)
64    ERC721(_name, _symbol)
65    VRFV2PlusWrapperConsumerBase(_vrfWrapper)
66{
67    _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
68
69    standardBaseURI = _standardBaseURI;
70    goldBaseURI = _goldBaseURI;
71
72    tokensPerBox = _tokensPerBox;
73    feeBPS = _feeBPS;
74    startTimestamp = _startTimestamp;
75
76    meme = IERC20(_meme);
77    unboxed = IUnboxed(_unboxed);
78    treasury = _treasury;
79}

Recommendation:

We advise some basic sanitization to be put in place by ensuring that each address specified is non-zero.

Alleviation:

All input arguments of the Boxed::constructor function are adequately sanitized as non-zero in the latest in-scope revision of the codebase, addressing this exhibit.

BDE-04S: Multiple Top-Level Declarations

Type	Severity	Location
Code Style		Boxed.sol:L13, L17

Description:

The referenced file contains multiple top-level declarations that decrease the legibility of the codebase.

Example:

Copy
13interface IUnboxed {
14    function mint(address to, uint256 tokenId, bool isGold) external;
15}
16
17contract Boxed is ERC721, ERC721Enumerable, AccessControl, ReentrancyGuard, VRFV2PlusWrapperConsumerBase {

Recommendation:

We advise all highlighted top-level declarations to be split into their respective code files, avoiding unnecessary imports as well as increasing the legibility of the codebase.

Alleviation:

The referenced top-level declaration(s) beyond the one that has the same name as the code file have been relocated to their dedicated file(s) (IUnboxed) and are instead imported as advised, increasing the code's clarity.

BDE-05S: Variable Mutability Specifiers (Constant)

Type	Severity	Location
Gas Optimization		Boxed.sol: • I-1: L30 • I-2: L48 • I-3: L49

Description:

The linked variables are assigned to only once during their own declaration.

Example:

Copy
30uint256 public mintLimit = 20;

Recommendation:

We advise them to be set as constant greatly optimizing their read-access gas cost.

Alleviation:

The mintLimit, numWords, and requestConfirmations contract-level variables have been set as constant, optimizing the code's gas cost significantly.

BDE-06S: Deprecated Native Asset Transfer

Type	Severity	Location
Language Specific		Boxed.sol:L126

Description:

The linked statement performs a low-level native asset transfer via the transfer function exposed by the address payable data type.

Impact:

As new EIPs such as EIP-2930 are introduced to the blockchain, gas costs can change and the transfer instruction of Solidity specifies a fixed gas stipend that is prone to failure should such changes be integrated to the blockchain the contract is deployed in. A prime example of this behaviour are legacy versions of Gnosis which were susceptible to this issue and would cause native transfers to fail if sent to a new address.

Example:

Copy
126payable(msg.sender).transfer(address(this).balance);

Recommendation:

We advise alternative ways of transferring assets to be utilized instead, such as OpenZeppelin's Address.sol library and in particular the sendValue method exposed by it. If re-entrancies are desired to be prevented based on gas costs, we instead advise a mechanism to be put in place that either credits an account with a native balance they can withdraw at a secondary transaction or that performs the native asset transfers at the end of the top-level transaction's execution.

Alleviation:

The Address::sendValue from the relevant OpenZeppelin dependency has now replaced the low-level transfer, alleviating this exhibit.

BDE-07S: Improper Invocations of EIP-20 transfer / transferFrom

Type	Severity	Location
Standard Conformity		Boxed.sol: • I-1: L113 • I-2: L116 • I-3: L150

Description:

The linked statements do not properly validate the returned bool values of the EIP-20 standard transfer & transferFrom functions. As the standard dictates, callers must not assume that false is never returned.

Impact:

If the code mandates that the returned bool is true, this will cause incompatibility with tokens such as USDT / Tether as no such bool is returned to be evaluated causing the check to fail at all times. On the other hand, if the token utilized can return a false value under certain conditions but the code does not validate it, the contract itself can be compromised as having received / sent funds that it never did.

Example:

Copy
113meme.transferFrom(msg.sender, address(this), tokensPerBoxAfterFee * _quantity);

Recommendation:

Since not all standardized tokens are EIP-20 compliant (such as Tether / USDT), we advise a safe wrapper library to be utilized instead such as SafeERC20 by OpenZeppelin to opportunistically validate the returned bool only if it exists in each instance.

Alleviation:

The SafeERC20 library of the OpenZeppelin dependency is now properly imported to the codebase and its SafeERC20::safeTransferFrom & SafeERC20::safeTransfer functions are correctly invoked in place of all potentially unhandled EIP-20 ERC20::transfer & ERC20::transferFrom invocations, addressing this exhibit.