Boxed Manual Review Findings

BDE-01M: Insecure Casting Operation

Type	Severity	Location
Logical Fault		Boxed.sol:L202

Description:

The Boxed::requestRandomWords function will insecurely cast the result of baseCallbackGas + (gasPerBox * _quantity) to a uint32 variable, permitting an overflow to occur trivially and thus an unfulfillable Chainlink randomness request to be created.

Impact:

The mainnet variant of the Boxed implementation charges users themselves for randomness requests rendering this particular issue to be solely harmful toward users who deliberately induce it.

Example:

Copy
199function requestRandomWords(uint256 _quantity) private returns (uint256) {
200    uint32 baseCallbackGas = 100000;
201    uint32 gasPerBox = 50000;
202    uint32 callbackGasLimit = uint32(baseCallbackGas + (gasPerBox * _quantity));

Recommendation:

We advise the code to perform a safe casting operation such as by utilizing OpenZeppelin's SafeCast library, ensuring that the result of the multiplication and addition can fit within the uint32 data type's upper bound (i.e. type(uint32).max).

Alleviation:

The SafeCast library by OpenZeppelin is now imported in the codebase and utilized to safely cast the resulting callback gas limit to the uint32 data type, alleviating this exhibit in full.

BDE-02M: Inconsistent Sanitization of Fee

Type	Severity	Location
Input Sanitization		Boxed.sol:L73, L185

Description:

The Boxed::constructor does not sanitize the configured _feeBPS value even though the Boxed::setFeeBPS function does so.

Impact:

An invalid fee can be configured for the system which would result in an inoperable Boxed implementation.

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 similar sanitization to be imposed across the two functions, ensuring that the system cannot be deployed with an invalid fee configuration.

Alleviation:

Proper sanitization of the configured feeBPS was introduced in the contract's Boxed::constructor, addressing this exhibit.

BDE-03M: Inexplicable Division Before Multiplication

Type	Severity	Location
Mathematical Operations		Boxed.sol: • I-1: L90 • I-2: L147

Description:

The referenced fee calculations will perform a division prior to a multiplication, resulting in an underestimation of fees due to mathematical truncation.

Impact:

Depending on whether the tokensPerBox configuration is wholly divisible by MAX_BPS or not, a truncation error would occur on all fee calculations.

Example:

Copy
90uint256 feePerBox = tokensPerBox / MAX_BPS * feeBPS;

Recommendation:

We advise the code to multiply before dividing, maximizing the accuracy of the calculations involved.

Alleviation:

The code was updated to properly perform a multiplication prior to a division, ensuring that the arithmetic operations possess the maximum accuracy possible.

BDE-04M: Significantly Inefficient & Limiting Randomness ID Association

Type	Severity	Location
Logical Fault		Boxed.sol:L94, L105, L123, L222, L227

Description:

The Boxed implementation will associate a Chainlink randomness request with the box IDs it is meant to apply to by keeping track of all IDs minted separately within an array, storing it to a mapping, and retrieving it when processing the randomness request.

We consider this approach to be severely limiting, prone to reaching the block gas limit, and overall inefficient.

Impact:

We consider the current implementation in a mainnet deployment scenario to be significantly limiting to the system's adoption as well as viability.

Example:

Copy
93/// @dev initialise array to store boxIds
94uint256[] memory boxIds = new uint256[](_quantity);
95
96/// @dev mint boxes
97for (uint256 i = 0; i < _quantity; i++) {
98    /// @dev increment tokenId counter
99    nextTokenId++;
100
101    _safeMint(msg.sender, nextTokenId);
102
103    if (_vrf) {
104        /// @dev add ID to array for later VRF verification
105        boxIds[i] = nextTokenId;
106
107        /// @dev track NFT IDs that have requested VRF
108        randomnessRequested[nextTokenId] = true;
109    }
110}
111
112/// @dev lock meme tokens in contract
113meme.transferFrom(msg.sender, address(this), tokensPerBoxAfterFee * _quantity);
114
115/// @dev send meme tokens fee to treasury
116meme.transferFrom(msg.sender, treasury, feePerBox * _quantity);
117
118if (_vrf) {
119    /// @dev request random number from VRF
120    uint256 requestId = requestRandomWords(_quantity);
121
122    /// @dev associate VRF request ID to newly minted NFTs 
123    requestIdToBoxIds[requestId] = boxIds;
124
125    /// @dev refund unused VRF ETH fees to caller
126    payable(msg.sender).transfer(address(this).balance);
127}

Recommendation:

As all boxing operations are sequential, we advise the code to store the start index at which tokens began minting as well as the amount of tokens, permitting two uint256 values to represent an arbitrary number of minting operations.

Alleviation:

The codebase was significantly refactored per our recommendation, ensuring a BoxRange is stored for VRF boxing requests and subsequently utilized to iterate through the number of boxes meant to be processed, alleviating this exhibit in full.