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

4 stars 2 forks source link

Decimal mishandling drains assets, causing unintended long-term value loss. #170

Closed c4-bot-6 closed 8 months ago

c4-bot-6 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L680-L693 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L680-L693

Vulnerability details

Impact

Mishandling decimals allow attackers to slowly drain additional assets over multiple deposits and withdrawals. This could extract substantial unintended value long-term.

Because in _convertIBTsToShares not using sufficient precision for decimal math: _convertIBTsToShares

function _convertIBTsToShares(
    uint256 _ibts,
    bool _roundUp
) internal view returns (uint256 shares) {
    (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);
    if (_ptRate == 0) {
        revert RateError();
    }

     // Not enough precision here
    shares = _ibts.mulDiv(
        _ibtRate,
        _ptRate,
        _roundUp ? Math.Rounding.Ceil : Math.Rounding.Floor
    );
}

The truncation happens here when the widow is discarded.

Proof of Concept

The _convertIBTsToShares math can lose precision and enable a slow drain attack stealing user funds over repeated deposits/withdrawals.

  1. Alice deposits 100 IBT with 18 decimals
  2. _convertIBTsToShares does: shares = ibts / ptRate
  3. The ptRate is 10^27 so dividing truncates a tiny IBT fraction
  4. Alice withdraws and slowly loses value each time

From truncating the result:

function _convertIBTsToShares(uint256 _ibts) internal view returns (uint256 shares) {

  shares = _ibts / ptRate;

}

Scenario

  1. Alice deposits 1 ETH over 1 year, with 0.001 ETH lost to truncation
  2. After 1000 deposits she has lost 1 ETH unintentionally

This succinctly conveys the risk of losing precision in _convertIBTsToShares.

Tools Used

Manual review

Recommended Mitigation Steps

Instead of storing ptRate with the implied 27 decimals, consider explicitly making it an integer with more decimal places (e.g., 36 decimals). This would reduce the amount discarded during truncation.

Assessed type

Decimal

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as duplicate of #5

c4-judge commented 8 months ago

JustDravee marked the issue as unsatisfactory: Invalid

c4-judge commented 8 months ago

JustDravee marked the issue as unsatisfactory: Invalid