hats-finance / StakeWise-0xd91cd6ed6c9a112fdc112b1a3c66e47697f522cd

Liquid staking protocol for Ethereum
Other
0 stars 0 forks source link

Incorrect Position's Fee Calculation is done on burnOsToken which is not in favor of the protocol #94

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @evokid Submission hash (on-chain): 0xd5b0fe1a050e72ec33ebdfda4236d9a277a8fe22bb06db1c690c698db4d9c308 Severity: high

Description: Description

Treasury fee is less than it should be in burnOsToken() function, if you look at it you will see that _osToken.burnShares() being called first then _syncPositionFee().

The proof is simple, on _osToken.burnShares() _totalAssets are being decreased which will affect the fee of the position definitely since profitAccrued in cumulativeFeePerShare() is returning the _unclaimedAssets().

Attack Scenario

Attachments

  1. Proof of Concept (PoC) On burnOsToken() L99-L104 a burn for osToken's shares is done then _syncPositionFee is called:
    assets = _osToken.burnShares(msg.sender, osTokenShares);

    // fetch user position
    OsTokenPosition memory position = _positions[msg.sender];
    if (position.shares == 0) revert Errors.InvalidPosition();
    _syncPositionFee(position);

If you check burnShares() you will see _totalAssets is being decreased L153:

      _totalAssets -= SafeCast.toUint128(assets);

Now the effect is here because _totalAssets is a factor to calculate cumulativeFeePerShare() which calls _unclaimedAssets() L336

function _unclaimedAssets() internal view returns (uint256) {
    // calculate time passed since the last update
    uint256 timeElapsed;
    unchecked {
      // cannot realistically underflow
      timeElapsed = block.timestamp - _lastUpdateTimestamp;
    }
    if (timeElapsed == 0) return 0;
    return Math.mulDiv(avgRewardPerSecond * _totalAssets, timeElapsed, _wad);
  }
}

We can see from the code above, when _totalAssets decreases, the returned value will be decreased too.

As Result on _syncPositionFee() profitAccrued will be decreased, Therefore treasury assets will decreased.

Recommendations: _syncPositionFee() should be called before _osToken.burnShares().

tsudmi commented 1 year ago

Issue looks similar to https://github.com/hats-finance/StakeWise-0xd91cd6ed6c9a112fdc112b1a3c66e47697f522cd/issues/96 Please check _osToken.burnShares() it does state update, so in _unclaimedAssets() timeElapsed will be 0 and statement if (timeElapsed == 0) return 0; will be executed