code-423n4 / 2024-02-spectra-findings

4 stars 2 forks source link

Users withdraw excess PT, draining sustainability; long-term accounting inaccuracy. #171

Closed c4-bot-3 closed 8 months ago

c4-bot-3 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/libraries/PrincipalTokenUtil.sol#L55-L70

Vulnerability details

Impact

Unwarranted PT Withdrawals. Users can withdraw more PT than entitled, draining sustainability funds during economic crashes.

Trigger: Negative rate exceeding >90% over short timeframe

Root Cause: Overflow in yield calculation _computeYield

function _computeYield(
    address _user,
    uint256 _userYieldIBT,
    uint256 _oldIBTRate,
    uint256 _ibtRate,
    uint256 _oldPTRate,
    uint256 _ptRate,
    address _yt
) external view returns (uint256) {
    if (_oldPTRate == _ptRate && _ibtRate == _oldIBTRate) {
        return _userYieldIBT;
    }
    uint256 newYieldInIBTRay;
    uint256 userYTBalanceInRay = IYieldToken(_yt).actualBalanceOf(_user).toRay(
        IYieldToken(_yt).decimals()
    );
    // ibtOfPT is the yield generated by each PT corresponding to the YTs that the user holds
    uint256 ibtOfPTInRay = userYTBalanceInRay.mulDiv(_oldPTRate, _oldIBTRate);
    if (_oldPTRate == _ptRate && _ibtRate > _oldIBTRate) {
        // only positive yield happened
        newYieldInIBTRay = ibtOfPTInRay.mulDiv(_ibtRate - _oldIBTRate, _ibtRate);

        // @audit-info This could overflow with >90% negative rate, assigning newYield a very high unintended value
}

Scenario: 1) Interest market crash causes IBT rate -95% 2) Overflow occurs assigning user 1000x actual yield 3) Attacker burns low-priced PT to redeem inflated yield

Then Incorrect Accounting

Impact: Inaccurate accounting undermines sustainability model

Likelihood: High given frequent rounding in conversions

Trigger: Gradual accumulation of small inaccuracies

Root Cause: Rounding errors during rate conversions:

function _computeYield(
    address _user,
    uint256 _userYieldIBT,
    uint256 _oldIBTRate,
    uint256 _ibtRate,
    uint256 _oldPTRate,
    uint256 _ptRate,
    address _yt
) external view returns (uint256) {
    if (_oldPTRate == _ptRate && _ibtRate == _oldIBTRate) {
        return _userYieldIBT;
    }
    uint256 newYieldInIBTRay;
    uint256 userYTBalanceInRay = IYieldToken(_yt).actualBalanceOf(_user).toRay(
        IYieldToken(_yt).decimals()
    );
    // ibtOfPT is the yield generated by each PT corresponding to the YTs that the user holds
    uint256 ibtOfPTInRay = userYTBalanceInRay.mulDiv(_oldPTRate, _oldIBTRate);
    if (_oldPTRate == _ptRate && _ibtRate > _oldIBTRate) {
        // only positive yield happened
        newYieldInIBTRay = ibtOfPTInRay.mulDiv(_ibtRate - _oldIBTRate, _ibtRate);

// @audit-info Rounding discards tiny dust each conversion

}

Scenario: 1) Over 2 years, 0.1% dust rounds off each day 2) Total loss exceeds 10% of reserves 3) Accounting now completely inaccurate

Proof of Concept

Users can steal funds by withdrawing more PT than entitled. Also, long term accounting inaccuracies undermine sustainability.

Tools Used

Manual review

Recommended Mitigation Steps

Instead of subtracting, consider comparing the sign and magnitude of _ibtRate and _oldIBTRate. If _ibtRate is significantly smaller, handle it as a special negative yield case.

Assessed type

Under/Overflow

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as insufficient quality report

gzeon-c4 commented 8 months ago

lack poc

c4-judge commented 8 months ago

JustDravee marked the issue as unsatisfactory: Invalid