Dria
Swan
NFTHardhat
21,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Statistics Calculations Are Vulnerable To Precision Loss

auditweiler

Summary

The statistics calculations relied upon in Dria are vulnerable to precision loss through integer truncation. In some cases, these precision losses are iteratively amplified.

Vulnerability Details

The Statistics library exports a number of operations which are susceptible to precision loss:

/// @notice Compute the mean of the data.
/// @param data The data to compute the mean for.
function avg(uint256[] memory data) internal pure returns (uint256 ans) {
uint256 sum = 0;
for (uint256 i = 0; i < data.length; i++) {
sum += data[i];
}
ans = sum / data.length;
}

https://github.com/Cyfrin/2024-10-swan-dria/blob/c8686b199daadcef3161980022e12b66a5304f8e/contracts/libraries/Statistics.sol#L6C5-L14C6

The implementation of the average calculation is nearly identical to what we would expect to see in an environment which supports floating point arithmetic.

However, the EVM does not provide a Floating Point Unit (FPU).

In this regard, applying the division operator can result in data truncation, demonstrated by the following toy example:

Welcome to Chisel! Type `!help` to show available commands.
➜ uint256[] memory data = new uint256[]();
data[0] = 100;
data[1] = 105;
data[2] = 102;
➜ avg(data)
Type: uint256
├ Hex: 0x0000000000000000000000000000000000000000000000000000000000000004
├ Hex (full word): 0x0000000000000000000000000000000000000000000000000000000000000004
└ Decimal: 102

In practicality, we'd expect (100 + 105 + 102) / 3 to equal 102.333333333. Depending upon the magnitude of values provided via data, precision losses could result in detrimental outcomes.

Even if the precision loss in avg were small, the implementation of variance conspires to magnify these:

/// @notice Compute the variance of the data.
/// @param data The data to compute the variance for.
function variance(uint256[] memory data) internal pure returns (uint256 ans, uint256 mean) {
@> mean = avg(data); /// @audit precision loss
uint256 sum = 0;
for (uint256 i = 0; i < data.length; i++) {
uint256 diff = data[i] - mean;
sum += diff * diff; /// @audit amplify precision loss per iteration
}
ans = sum / data.length; /// @audit precision loss
}

Here, we can see that even a small initial precision losses in the avg function will be amplified proportionally to the length ofdata, since this error is taken into account for the square of diff.

We can also see that a final round of precision loss is also applied before returning the result.

Impact

Loss of precision resulting in incorrect consensus outcomes.

Tools Used

Manual Review

Recommendations

Broadly, we recommend applying decimal precision when making statistics calculations, i.e.:

+ uint256 internal constant WAD = 10 ** 18;
+
/// @notice Compute the mean of the data.
/// @param data The data to compute the mean for.
function avg(uint256[] memory data) internal pure returns (uint256 ans) {
uint256 sum = 0;
for (uint256 i = 0; i < data.length; i++) {
sum += data[i];
}
- ans = sum / data.length;
+ ans = sum * WAD / data.length;
}

However, for Dria we contend that prb-math would be especially beneficial, due to its optimization for scientific calculations:

https://github.com/PaulRBerg/prb-math

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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