Dria

Summary

The calculations within Statistics::variance used in the LLMOracleCoordinator.sol can fail due to an underflow issue when scores contain values smaller than the mean. This leads to reverts during the finalizeValidation process, causing a denial of service (DoS) for tasks requiring validation. As a result, tasks could remain stuck in a PendingValidation state, preventing proper finalization and subsequent task completion.

Description

As the protocol documentation states "For each oracle request, we expect a number of generations and a number of validations afterwards. The validations will return a score for each generation......"
These scores are numerical values that measure the quality or accuracy of the generated outputs. The exact range of these scores isn't however specified in the documentation. Note that in the test files the score is determined using a helper function parseEther, which basically converts the decimal string to a BigInt with 18 decimal places, e.g. parseEther("0.9") is converted to 900000000000000000. However, the scores can be any valid uint256 numbers. For simplicity we will work with smaller uint256 numbers, in the range 0 to 5.
For example, a score of 5 might indicate a very good answer, while a score of 0 might indicate an inadequate answer.

Now lets assume that scores[] array in LLMOracleCoordinator::finalizeValidation (see [https://github.com/Cyfrin/2024-10-swan-dria/blob/main/contracts/llm/LLMOracleCoordinator.sol#L331]) contains 5 values, e.g. [2,3,3,4,3]. In the next step the function will compute the mean and standard deviation, using Statistics.stddev(scores), see [https://github.com/Cyfrin/2024-10-swan-dria/blob/main/contracts/llm/LLMOracleCoordinator.sol#L335]. After the Statistics.stddev(scores) is called the first what will be computed is the mean of the data using Statistics::avg:

If we stick to our example array [2,3,3,4,3] the Statistics::avg would return for the variable ans the value of 3.
The returned value would be than used in Statistics::variance:

Line 22 in Statistics.sol is crucial (see [https://github.com/Cyfrin/2024-10-swan-dria/blob/main/contracts/libraries/Statistics.sol#L22]), as with the provided values [2, 3, 3, 4, 3], it leads to an underflow. Specifically, every score value in the data array that is smaller than the computed mean would break the function's logic. This underflow causes the compiler to revert the entire transaction, leading to a denial of service (DoS) in LLMOracleCoordinator::finalizeValidation. Consequently, the latest call to LLMOracleCoordinator::validate would also fail, as only the last validator gets to call finalizeValidation. Since the scores pushed into the TaskValidation array of structs by previous validators can no longer be modified, this issue would result in a task not being able to be set to a Completed state.

PoC

For the proof of concept we will stick to the score values, similar to those in LLMOracleCoordinator.test.ts. Here is the test case and setup, please paste it into the LLMOracleCoordinator.test.ts file and adjust the this.beforeAll(async function () section with additional validators, like this:

The test can be run with yarn test ./test/LLMOracleCoordinator.test.ts.
Here are the logs:

As you can see from the error logs, the transaction failed with an error message: panic code 0x11 (Arithmetic operation overflowed outside of an unchecked block) and the task status remained at 2 (PendingValidation).

Impact

If LLMOracleCoordinator::finalizeValidation reverts while being called from within the LLMOracleCoordinator::validate function, then the entire execution of the validate function will also revert. That would mean that the latest validation that was pushed into TaskValidation array of structs will be removed (due to the rollback) and the task's status will not change to Completed. Since the scores pushed by previous validators to the TaskValidation array of structs cannot be changed, tasks may become permanently stuck in the PendingValidation status, resulting in a DoS vulnerability.

Tools Used

Manual Code Review, Hardhat

Recommendations

One of the possible solutions would be to refactor the variance function to handle the signed differences between each data point and the mean. This can be achieved using an integer-based signed calculation. Consider making the following changes to the variance function:

If you paste this modified implementation into Statistics::variance and adjust the last line in the PoC to expect(finalRequest.status).to.equal(TaskStatus.Completed); and run the test with yarn test ./test/LLMOracleCoordinator.test.ts it will pass.