Dria
Swan
NFTHardhat
21,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Statistics.sol underflows when mean score exceeds at least one element

jsondoge

Summary

Due to faulty unsigned math in Statistics.sol variance() - calling variance() or stddev() have a high likelihood of causing an underflow panic.

Vulnerability Details

variance() function in Statistics has a flaw due to the following subtraction:

function variance(uint256[] memory data) internal pure returns (uint256 ans, uint256 mean) {
mean = avg(data);
uint256 sum = 0;
for (uint256 i = 0; i < data.length; i++) {
uint256 diff = data[i] - mean; // <- bug source
sum += diff * diff;
}
ans = sum / data.length;
}

This means that any time the "mean" output from avg() exceeds any of the used elements in the data array - the output will fall below 0 and cause uint256 to underflow. And because the main goal of Statistics library is to calculation deviation, as soon as there is going to be one, the function will have a high likelihood of failure.

Impact

Statistics is used only in finalizeValidation() and the problem can be avoided if:

  • validation disabled or only one validator choosen for the task

  • all validator scores do not deviate from each other by more than 1 (there are minor exceptions if most elements are low and equal example: 1, 1, 1, 3 - avg will be 1 and not underflow)

But anything else than the above conditions and the calculations will underflow. This will cause the last oracle to unable to finalizeValidation - loss of gas and
the buyerAgent will not be able to get a getBestResponse due the task never reaching "Completed" status - loss of fees.

Tools Used

Manual review + hardhat tests

Test is a modified "LLMOracleCoordinator.test.ts" - describe("With validation"). Simply replace dummy scores from exactly the same, to any larger deviating ones

describe("with validation", function () {
const [numGenerations, numValidations] = [2, 2];
const dummyScore = parseEther("0.9"); // don't remove as used for asserts later
- const scores = Array.from({ length: numGenerations }, () => dummyScore);
+ const scores = [parseEther("0.9"), parseEther("0.8")];

As the tests depend on eachother, they will start failing at:

LLMOracleCoordinator
✔ should register oracles (44ms)
with validation
✔ should make a request
✔ should respond (1/2 & 2/2)
✔ should NOT respond if task is not pending generation
✔ should NOT validate if not a registered Oracle
✔ should validate (1/2) a generation only once
1) should validate (2/2) // <- starts failing due to finalizeValidation being called
2) should see generation scores // <- all other crash and burn due to dependency on each other
3) should see rewards
...

The stack trace of that test looks like this:

should validate (2/2):
Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation overflowed outside of an unchecked block)
at LLMOracleCoordinator.variance (contracts/libraries/Statistics.sol:22)
at LLMOracleCoordinator.stddev (contracts/libraries/Statistics.sol:32)
at LLMOracleCoordinator.finalizeValidation (contracts/llm/LLMOracleCoordinator.sol:366)
at LLMOracleCoordinator.validate (contracts/llm/LLMOracleCoordinator.sol:305)
...

We can see it starts at Statistics.sol:22 which is the exact line of the "mean" subtraction.

Recommendations

Take into account that for some elements it can be negative - add explicit handling. Example solution:

uint256 diff = data[i] >= mean ? data[i] - mean : mean - data[i];
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Underflow in computing variance

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.