Swan Protoocl contest || NFTs, AI, Statistics || 25 Oct 2024 to 05 Nov 2025 on Codehawks
My Finding Suppary
| ID | Title | Severity |
|---|---|---|
| H‑01 | finalizeValidation() will fail if Standard Deviation is greater than Average |
HIGH |
| H-02 | Calculating variance() will fail if the mean is greater than one of the values |
HIGH |
| M‑01 | Update state requests or Purchase requests occurring at the end of the phase will not process | MEDIUM |
| M-02 | withdrawing platform fees takes Validators/Generators Fees, as well as money paid for incompleted tasks | MEDIUM |
| M-03 | Validators can grief Generators and hurt users in case of 1 validation only |
MEDIUM |
| L-01 | Tasks with no validations will always take the first generator result whatever it was | LOW |
When finalizing the validation, we are rewarding only the valid validators and neglect outliers. We do this by checking that the score lies in the boundaries of the mean +/- standard deviation. This behaviour is famous in statistics to determine Outliers, etc...
The problem is that we are doing the lower outliers check (mean - Standard Deviation), without checking if the Standard Deviation is smaller than mean (average) or not.
llm/LLMOracleCoordinator.sol#L343 | llm/LLMOracleCoordinator.sol#L368
function finalizeValidation(uint256 taskId) private {
...
for (uint256 g_i = 0; g_i < task.parameters.numGenerations; g_i++) {
...
for (uint256 v_i = 0; v_i < task.parameters.numValidations; ++v_i) {
uint256 score = scores[v_i];
>> if ((score >= _mean - _stddev) && (score <= _mean + _stddev)) { ... }
}
...
}
...
for (uint256 g_i = 0; g_i < task.parameters.numGenerations; g_i++) {
// ignore lower outliers
>> if (generationScores[g_i] >= mean - generationDeviationFactor * stddev) {
_increaseAllowance(responses[taskId][g_i].responder, task.generatorFee);
}
}
}There are two instances here, checking lower outliers of Validators, and checking lower outliers of Generators. Both checks is not handling the case if Standard Deviation is greater than mean or not. Which will make finalization process revert if it occuar.
Another thing here is that We are using Factory Formula generationDeviationFactor, And by increasing the generator factory we are accepting more space of errors, making the underflow issue likelihood increase.
Make the lower outlier boundary equal zero in case of Standard Deviation is greater than mean.
diff --git a/contracts/llm/LLMOracleCoordinator.sol b/contracts/llm/LLMOracleCoordinator.sol
index 1ba2176..269e8be 100644
--- a/contracts/llm/LLMOracleCoordinator.sol
+++ b/contracts/llm/LLMOracleCoordinator.sol
@@ -340,7 +340,8 @@ contract LLMOracleCoordinator is LLMOracleTask, LLMOracleManager, UUPSUpgradeabl
uint256 innerCount = 0;
for (uint256 v_i = 0; v_i < task.parameters.numValidations; ++v_i) {
uint256 score = scores[v_i];
- if ((score >= _mean - _stddev) && (score <= _mean + _stddev)) {
+ uint256 lowerBoundry = _stddev <= _mean ? _mean - _stddev : 0
+ if ((score >= lowerBoundry) && (score <= _mean + _stddev)) {
innerSum += score;
innerCount++;
@@ -365,7 +366,8 @@ contract LLMOracleCoordinator is LLMOracleTask, LLMOracleManager, UUPSUpgradeabl
(uint256 stddev, uint256 mean) = Statistics.stddev(generationScores);
for (uint256 g_i = 0; g_i < task.parameters.numGenerations; g_i++) {
// ignore lower outliers
- if (generationScores[g_i] >= mean - generationDeviationFactor * stddev) {
+ uint256 lowerBoundry = generationDeviationFactor * stddev <= mean ? mean - generationDeviationFactor * stddev : 0
+ if (generationScores[g_i] >= lowerBoundry) {
_increaseAllowance(responses[taskId][g_i].responder, task.generatorFee);
}
}When calculating the Variant of a set of variants, we add the summation of the square root of the difference between each element and the Average then divide them by their number
When Subtracting The element by the Average, we made a Power 2 operation, which makes the value positive even if the result was negative.
The problem is that we are performing the subtraction operation first (Element - Average), and put it in uint256, which will make it revert because of underflow in solidity.
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;
sum += diff * diff;
}
ans = sum / data.length;
}This will make calculating variance() always revert if one of the elements is smaller than the average.
This will make finalizeValidation() process revert as it uses stddev() function that uses variance() functions for the scores input
If Element < Avg reverse the order of subtraction
diff --git a/contracts/libraries/Statistics.sol b/contracts/libraries/Statistics.sol
index 8c53643..ac713c9 100644
--- a/contracts/libraries/Statistics.sol
+++ b/contracts/libraries/Statistics.sol
@@ -19,7 +19,7 @@ library Statistics {
mean = avg(data);
uint256 sum = 0;
for (uint256 i = 0; i < data.length; i++) {
- uint256 diff = data[i] - mean;
+ uint256 diff = data[i] >= mean ? data[i] - mean : mean - data[i];
sum += diff * diff;
}
ans = sum / data.length;[M-01] Update state requests or Purchase requests occurring at the end of the phase will not process
BuyerAgent can make two requests either purchase request or StateUpdate request.
First, he should make a BuyerAgent::oraclePurchaseRequest() request to buy all the items he needs, then call BuyerAgent::purchase() after His task gets completed by Oracle coordinator to buy the items he wants.
Then, in case of buying new items success he should make BuyerAgent::oracleStateRequest() to update his state after buying items, then call BuyerAgent::updateState() to change his state.
The problem here is that there is no check when BuyerAgent::oraclePurchaseRequest() or BuyerAgent::oracleStateRequest() get requested. There is just a check that enforces firing both of them on a given Phase.
We will explain the problem in the purchasing process, but it also existed in updating the state process.
When requesting to purchase items, we check that we are at a Round that is at Buy Phase, and when doing the actual purchase the Round should be the same as the Round we call BuyerAgent::oraclePurchaseRequest() as well as the phase should be Buy.
swan/BuyerAgent.sol#L189-L195 | swan/BuyerAgent.sol#L222-L230
function oraclePurchaseRequest(bytes calldata _input, bytes calldata _models) external onlyAuthorized {
// check that we are in the Buy phase, and return round
>> (uint256 round,) = _checkRoundPhase(Phase.Buy);
>> oraclePurchaseRequests[round] =
swan.coordinator().request(SwanBuyerPurchaseOracleProtocol, _input, _models, swan.getOracleParameters());
}
// -------------------
function purchase() external onlyAuthorized {
// check that we are in the Buy phase, and return round
>> (uint256 round,) = _checkRoundPhase(Phase.Buy);
// check if the task is already processed
uint256 taskId = oraclePurchaseRequests[round];
>> if (isOracleRequestProcessed[taskId]) {
revert TaskAlreadyProcessed();
}
}For BuyerAgent::purchase() to process, we should be at the same Round as well as at the Phas, which is Buy in that example, when we fired BuyerAgent::oraclePurchaseRequest().
the flow is as follows:
- BuyerAgent::oraclePurchaseRequest()
- Generators will generate output in LLM Coordinator
- Validators will validate in LLM Coordinator
- Task marked as completed
- BuyerAgent::purchase()
There is time will be taken for generators and validators to make the output (complete the task).
So if BuyerAgent::oraclePurchaseRequest gets fired before the end of Buying Phase with little time, this will make the Buy ends and enters Withdraw phase before Generators and Validaotrs Complete that task, resulting in losing Fees paid by the BuyerAgent when requesting the purchase request.
- BuyerAgent wants to buy a given item
- His Round is
10and we are at the Buy phase know - The buying phase is about to end there is just one Hour left
- BuyerAgent Fired
BuyerAgent::oraclePurchaseRequest() - Fees got paid and we are waiting for the task to complete
- Generators and Validators took 6 Hours to complete this task
- Know, the BuyerAgent Round is
10and the Phase isWithdraw - calling
BuyerAgent::purchase()will fail as we are not in theBuyPhase
The problem is that there is no time left for requesting and firing, if the request occur at the end of the Phase, finalizing the request either purach or update state will fail, as the phase will end.
We are doing Oracle and off-chain computations for the given task, and the time to finalize (complete) a task differs from one task to another according to difficulty.
There are three things here that make this problem occur.
- If the Operator is not active, the
BuyerAgentshould call the request himself. - If Completing the Task process takes too much time, this can occur for Tasks that require a lot of validations, or difficulty is high
- if there is a High Demand for a given request the Operator may finalize some of them at the end.
Don't allow requests for all the phase ranges.
For example, In case we Have 7 days for Buy phase, we should Stop requesting purchase requests at the end of 2 days to not make requests occur at the last period of the phase resulting in an insolvable state if it gets completed after 2 days.
This is just an example. The period to stop requested should be determined according to the task itself (num of Generators/Validators needed and its difficulty).
[M-02] withdrawing platform fees takes Validators/Generators Fees, as well as money paid for incompleted tasks
When doing tasks there are 3 types of fees that the caller should pay.
- Platform Fees
- Generator Fees
- Validator Fees
llm/LLMOracleManager.sol#L110-L120
function getFee(LLMOracleTaskParameters calldata parameters)
public
view
returns (uint256 totalFee, uint256 generatorFee, uint256 validatorFee)
{
uint256 diff = (2 << uint256(parameters.difficulty));
generatorFee = diff * generationFee;
validatorFee = diff * validationFee;
totalFee =
platformFee + (parameters.numGenerations * (generatorFee + (parameters.numValidations * validatorFee)));
}When taking platform Fees we are taking all money in the contract.
llm/LLMOracleCoordinator.sol#L375-L377
function withdrawPlatformFees() public onlyOwner {
feeToken.transfer(owner(), feeToken.balanceOf(address(this)));
}When Completing The Task using finalizeValidation(), or when completing tasks that require no validation. We are not sending Fees directly to the Oraacle Registry, we are increasing there allowance so that they can transfer their tokens themselves.
llm/LLMOracleCoordinator.sol#L348 | llm/LLMOracleCoordinator.sol#L369
function finalizeValidation(uint256 taskId) private {
...
for (uint256 g_i = 0; g_i < task.parameters.numGenerations; g_i++) {
...
for (uint256 v_i = 0; v_i < task.parameters.numValidations; ++v_i) {
uint256 score = scores[v_i];
if ((score >= _mean - _stddev) && (score <= _mean + _stddev)) {
...
>> _increaseAllowance(validations[taskId][v_i].validator, task.validatorFee);
}
}
...
}
...
(uint256 stddev, uint256 mean) = Statistics.stddev(generationScores);
for (uint256 g_i = 0; g_i < task.parameters.numGenerations; g_i++) {
// ignore lower outliers
if (generationScores[g_i] >= mean - generationDeviationFactor * stddev) {
>> _increaseAllowance(responses[taskId][g_i].responder, task.generatorFee);
}
}
}finalizeValidation() will only get fired when completeing the task. But for the caller pays the fees when calling Coordinator::request(), so the fees paid for tasks that didn't yet completed, will also get transfered to the Owner as PlatformFees.
So in brief.
- When withdrawing Platform Fees All money in the contract will get transferred to the owner.
- Validators/Generators that didn't withdraw there money (have allowance), will lose their money
- fees paid for tasks that didn't complete yet but are either in
PendingGenerationorPendingValidation, will also get transferred to the owners.
This is incorrect Fees accumulating logic, which will result in an unfair process for validators and Generators when distributing Fees.
Another thing is that there is no way to know exactly what our Validadators or Generators need. Admins will have to query over all there Registered addresses, which is open to anyone. And there are no Events, nor an array that groups them. making the process literally impossible for them to pay the money they own to Validators and Generators.
Accumulate PlatformFees on a global variable, in addition to Fees for outliers, and when withdrawing take this value only, instead of Taking all contract balance.
When requesting for tasks, the caller can determine the number of Validators that validate the generations.
The validation process is done using the Standard deviation method, where the values that are far from the mean are ignored.
And the score for a given generation is given by the average of the scores make by validators.
The problem is That the Standard Deviation method, is not a checking mechanism to validate a small number of inputs, validating one input is non-sense, as this single validator can give any score to any generation, and no one will catch him.
So in case we have 5 generations and 1 validator, and there are 1 Generator OutLier. The validator can make scores for the valid generations' mad ones, and the outlier gives him the ideal score allowing him to prevent honest generators from their rewards, as well as hurting BuyerAgent by giving the wrong response to him.
Oracles are accessible to anyone, and the Validation process is made to reach what is like a consyses and detect outliers, but accepting 1 validator will make the validation process inactive, and the 2 validations also can be ineffective a all.
Make the number of validations either 0 if no validations, or in case of accepting validations, accept only 3 or higher.
To get the bestResponse we compare the score for generation, and take the best score.
llm/LLMOracleCoordinator.sol#L413-L422
function getBestResponse(uint256 taskId) external view returns (TaskResponse memory) {
...
TaskResponse storage result = taskResponses[0];
uint256 highestScore = result.score;
for (uint256 i = 1; i < taskResponses.length; i++) {
>> if (taskResponses[i].score > highestScore) {
highestScore = taskResponses[i].score;
result = taskResponses[i];
}
}
return result;
}The score value in generator responses, is assigned by Validators. but for no validators for that task, all scores will be zero, making always the first generator output is the best one.
This will make other generations useless, and nonsense to the task requester.
We think that making a task with no validations only requires one generator as it will always be the output. Or there may be another method for picking the best response at this case.