RandomNumberGenerator.sol

[MEDIUM] Missing Request Validation in _convertRandomNumberToArray()

The _convertRandomNumberToArray function accesses pendingRequests[_requestId].numberCount without validating that the request exists. If _requestId is invalid, _numberCount will be 0 (default uint value), causing the function to create an empty array unnecessarily. The error is only caught later in _returnRandomNumbers() at line 84, wasting gas on array allocation.

function _convertRandomNumberToArray(uint _requestId, uint _randomNumber) internal view returns (uint[] memory randomNumbers_) {
    uint _numberCount = pendingRequests[_requestId].numberCount; // No validation that request exists
    randomNumbers_ = new uint[](_numberCount); // Creates empty array if _numberCount == 0
    // ...
}

Recommended Solution

This issue should be resolved per-protocol integration, as they will handle reverts and erroneous requests differently. Most cases should only respond with a requestId that has already been cached within the contract. However, we would recommend that checks are put in place in the various callback functions to detect this.

[MEDIUM] Zero numberCount Edge Case Not Handled

If _numberCount is 0, the function creates an empty array without validation. While this will eventually fail in _returnRandomNumbers() at line 89, it should be caught earlier to save gas and provide clearer error messages.

uint _numberCount = pendingRequests[_requestId].numberCount;
randomNumbers_ = new uint[](_numberCount); // Creates empty array if _numberCount == 0

Recommended Solution

uint _numberCount = request.numberCount;

// Validate that numberCount is greater than 0
if (_numberCount == 0) {
  revert InvalidNumberCount(_requestId);
}

randomNumbers_ = new uint[](_numberCount);

[LOW] block.number Dependency Reduces Entropy

_convertRandomNumberToArray uses block.number in the hash generation. As numbers are generated in the same block, this makes the block.number being passed as value pointless, and reduces entropy across different requests. Uniqueness is still maintained through _randomNumber and i parameters.

Recommended Solution

Consider using block.timestamp or blockhash(block.number - 1) for additional entropy, though current implementation would be viable with block.number simply removed.

randomNumbers_[i] = uint(keccak256(abi.encodePacked(_randomNumber, block.timestamp, i)));

[LOW] Use of abi.encodePacked

The _convertRandomNumberToArray function uses abi.encodePacked() which can have collision risks in certain edge cases.

However, in this specific usage with uint256 values (_randomNumber, block.number, i), collisions are extremely unlikely.

We would recommend using abi.encode() as it is more future-proof.

Recommended Solution

Consider using abi.encode() for better type safety, though it will increase gas costs slightly:

randomNumbers_[i] = uint(keccak256(abi.encode(_randomNumber, block.number, i)));