`BribeFactoryUpgradeable::addRewards` lacks array length match check

[hats-bug-reporter[bot]]

Github username: @inallhonesty Twitter username: 0xInAllHonesty Submission hash (on-chain): 0xd97390b5cf432d4724f7f38ecc7cda0a18375c86d3f6d27513603dbf4220fda5 Severity: low

Description: addRewards never checks the length of the input arrays, leading to situation where some of the tokens are ignored:

    function addRewards(address[][] memory _token, address[] memory _bribes) external {
        require(msg.sender == voter || msg.sender == owner(), "only voter or owner");

        for (uint256 i; i < _bribes.length; ) {
            IBribe(_bribes[i]).addRewardTokens(_token[i]);
            unchecked {
                i++;
            }
        }
    }

If the _token.length > _bribes.length (let's say first is 5 the last is 3) then rewards at index 3 and 4 are ignored by the for loop, thus the line IBribe(_bribes[i]).addRewardTokens(_token[i]); won't cover them. If the _token.length > _bribes.length (let's say first is 30 the last is 15) the function will fail, wasting a decent amount of gas.

Recommendation: Implement a check that forces the imput arrays length to match.

    function addRewards(address[][] memory _token, address[] memory _bribes) external {
++    require(_bribes.length == _token.length, "only arrays with matching length)
        require(msg.sender == voter || msg.sender == owner(), "only voter or owner");

        for (uint256 i; i < _bribes.length; ) {
            IBribe(_bribes[i]).addRewardTokens(_token[i]);
            unchecked {
                i++;
            }
        }
    }

[BohdanHrytsak]

Thank you for the submission.

Falls under OOS as a possible misconfiguration problem

[inallhonesty]

Falls under OOS as a possible misconfiguration problem

Can you please point me towards where this is defined? I'm asking because reading the things that don't qualify as low I found this:

LIMITATIONS: Reporters will not receive a bounty for:

Any known issue, such as:
Issues mentioned in any previous audit reports
Vulnerabilities that were already made public (either by HATS or by a third party)
“Centralization risks” that are known and/or explicitly coded into the protocol (e.g. an administrator can upgrade crucial contracts and steal all funds)
Attacks that require access to leaked private keys or trusted addresses.
Issues that are not responsibly disclosed (issues should typically be reported through our platform)

What I've reported doesn't fall under any of the things mentioned above.

One of the criterion for low is Smart contracts fail to work correctly, but no funds are at risk. Even if the funds are not at risk, this contract will not work properly with arrays that have length missmatch.

[BohdanHrytsak]

@inallhonesty I see that there were some problems with what was in the scope and what was out of scope and with the links in the Fenix competition announcement ((

When reviewing, I was guided by the fact that this method has call restrictions and a problem such as a transaction failure will depend only on the set input parameters or owner error.

https://github.com/Satsyxbt/Fenix?tab=readme-ov-file#publicly-known-issues

[inallhonesty]

If the _token.length > _bribes.length the transaction won't fail, it will be executed in a manner not intended by the voter or owner. Part of their input will simply be ignored. The problem is it will go under the radar like nothing happened. This is why I think it's a valid low.