BoxedL2 Manual Review Findings

BL2-01M: Inconsistent Sanitization of Fee

Type	Severity	Location
Input Sanitization		BoxedL2.sol:L74, L166

Description:

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

Impact:

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

Example:

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

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 BoxedL2::constructor, addressing this exhibit.

BL2-02M: Inexplicable Division Before Multiplication

Type	Severity	Location
Mathematical Operations		BoxedL2.sol: • I-1: L87 • I-2: L131

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
87uint256 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.

BL2-03M: Insecure Casting Operation

Type	Severity	Location
Logical Fault		BoxedL2.sol:L191

Description:

The BoxedL2::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:

A malicious user could exploit this overflow to create multiple unfulfillable Chainlink randomness requests and thus expend the Chainlink allocation of the Box Fun in the layer 2 deployment.

Example:

Copy
187/// @param _quantity is the amount of boxes being minted - helps calculate callbackGasLimit
188function requestRandomWords(uint256 _quantity) private returns (uint256) {
189    uint32 baseCallbackGas = 100000;
190    uint32 gasPerBox = 50000;
191    uint32 callbackGasLimit = uint32(baseCallbackGas + (gasPerBox * _quantity));
192
193    uint256 requestId = s_vrfCoordinator.requestRandomWords(
194        VRFV2PlusClient.RandomWordsRequest({
195            keyHash: keyHash,
196            subId: subscriptionId,
197            requestConfirmations: requestConfirmations,
198            callbackGasLimit: callbackGasLimit,
199            numWords: numWords,
200            extraArgs: VRFV2PlusClient._argsToBytes(
201                VRFV2PlusClient.ExtraArgsV1({
202                    /// @dev pay fee in native token
203                    nativePayment: true
204                })
205            )
206        })
207    );
208
209    return requestId;
210}

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.

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

Type	Severity	Location
Logical Fault		BoxedL2.sol:L91, L101, L114, L216

Description:

The BoxedL2 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 to incur significant gas costs redundantly and to potentially limit the maximum number of NFTs mintable due to breaching the block's gas limit earlier than expected.

Example:

Copy
82function box(uint256 _quantity) external nonReentrant {
83    require(block.timestamp >= startTimestamp, "Boxing not yet started");
84    require(_quantity > 0 && _quantity <= mintLimit, "Invalid mint quantity");
85    require(nextTokenId + _quantity <= MAX_SUPPLY, "Max supply reached");
86
87    uint256 feePerBox = tokensPerBox / MAX_BPS * feeBPS;
88    uint256 tokensPerBoxAfterFee = tokensPerBox - feePerBox;
89
90    /// @dev initialise array to store boxIds
91    uint256[] memory boxIds = new uint256[](_quantity);
92
93    /// @dev mint NFTs
94    for (uint256 i = 0; i < _quantity; i++) {
95        /// @dev increment tokenId counter
96        nextTokenId++;
97
98        _safeMint(msg.sender, nextTokenId);
99
100        /// @dev add ID to array for later VRF verification
101        boxIds[i] = nextTokenId;
102    }
103
104    /// @dev lock meme tokens in contract
105    meme.transferFrom(msg.sender, address(this), tokensPerBoxAfterFee * _quantity);
106
107    /// @dev send meme tokens fee to treasury
108    meme.transferFrom(msg.sender, treasury, feePerBox * _quantity);
109
110    /// @dev request random number from VRF
111    uint256 requestId = requestRandomWords(_quantity);
112
113    /// @dev associate VRF request ID to newly minted NFTs 
114    requestIdToBoxIds[requestId] = boxIds;
115}

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.