code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

PRBMATH `pow()` function can return inconsistent values which is root of computation for the Area under curve`c(d) = −t ∗ ln(α)∗α ^ d * ` #395

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/PaulRBerg/prb-math/blob/df27d3d12ce12153fb166e1e310c8351210dc7ba/src/sd59x18/Math.sol#L596-L609 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/DrawAccumulatorLib.sol#L390-L399

Vulnerability details

Impact

Protocol is highly dependent on the computation of area under the curve c(d) = −t ∗ ln(α)∗α ^ d which uses PRB-Math's Pow() function to calculate . Recently It has been found this pow() function can give inconsistent values and break the computation.

PRBMath's pow() function takes two signed integer (say a and b) and returns ab A proper implementation of power function states that : for any a >= b and x >= 0 , a x >= b ** x

Simplifying this further as (a>=b)= ((a/b)>=1) and so (a/b)**x >=1 [x>=0] this pow() function doesn't follow this and gives inconsistent values.

This Mainly affects integrate function of DrawAccumulatorLib.sol which integrate from a lower value higher value for the exponential weighted average

Proof of Concept

File: prb-math/src/sd59x18/Math.sol

596: function pow(SD59x18 x, SD59x18 y) pure returns (SD59x18 result) {
    int256 xInt = x.unwrap();
    int256 yInt = y.unwrap();

    if (xInt == 0) {
        result = yInt == 0 ? UNIT : ZERO;
    } else {
        if (yInt == uUNIT) {
            result = x;
        } else {
            result = exp2(mul(log2(x), y));
        }
    }
609: }

Test Function

function testPow() public {
uint128 high = 505456470057136353;
uint128 low = 505456461792312955;
assert(high > low);
SD59x18 exp = UD2x18.wrap(31414735).intoSD59x18();
SD59x18 highPow = high.intoSD59x18().pow(exp);
SD59x18 lowPow = low.intoSD59x18().pow(exp);
assert(SD59x18.unwrap(highPow) < SD59x18.unwrap(lowPow));
}

Root cause described by a senior watson - "in the exp2() function, bit masks are applied to multiply the result by root(2,2^-i) for each position i with a value of 1. However at Common.sol#L170, the value is incorrectly included as 0xFF00000000 instead of 0xFF000000 "

Tools Used

Manual , Foundry Testing

Recommended Mitigation Steps

Ensure that calculation is accurate by adjusting the comparison value in exp2() function

Assessed type

Math

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #423

Picodes commented 1 year ago

Downgrading to Med as this report doesn't show how it could qualify for High severity

c4-judge commented 1 year ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

Picodes marked the issue as selected for report

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

catellaTech commented 1 year ago

Hello, Picodes! While the issue with that function is real, I believe the appropriate mitigation was proposed by Obront you can see more about this in this post , as the co-founder of the project being audited is also the creator of PRBMath. Therefore, it seems out of reach for the PT team to fix the function directly, and instead, the suitable mitigation would be the one proposed by the PRBMath team after they resolved the problem with exp2(). I discussed this further in my report.

Additionally, I think this issue should be considered of high severity because even though the problem was with exp2(), there were some inconsistencies in functions like div and mul, which are heavily used in important calculations within PT. The PRBMath team has already resolved these inconsistencies in version 4.

c4-judge commented 1 year ago

Picodes marked issue #423 as primary and marked this issue as a duplicate of 423

Picodes commented 1 year ago

@catellaTech thank you for your comment. You are correct about the mitigation. Regarding the severity, I downgraded these issues to Med because in my opinion none of these reports properly demonstrates that this could lead to a credible loss of funds scenario. From my understanding, the issue on pow occurs only in a specific range (it is related to a bitmask error and required 50000 fuzz tests to be uncovered), so there is a significant probability that the affected zone cannot be touched by DrawAccumulatorLib.sol TierCalculationLib.sol, or that the resulting inconsistencies aren't large enough to lead to a loss of funds. Overall I think Medium severity is more appropriate here - although if we dig into it the issue might be High.

catellaTech commented 1 year ago

Thanks so much for the answer! 😊👍

asselstine commented 1 year ago

Fixed here: https://github.com/GenerationSoftware/pt-v5-prize-pool/commit/ba56ea8bac3bce06f1e08ae071a19954dd720b1f