_checkOsTokenPosition does not really update position because memory is used instead of storage

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0x53819d9a186e4f089e497b06644b7d946345a3b9d0a463cb6e4547c0c8c12a37 Severity: high

Description: Description\

_checkOsTokenPosition does not really update position

Attack Scenario\

https://github.com/stakewise/v3-core/blob/c82fc57d013a19967576f683c5e41900cbdd0e67/contracts/vaults/modules/VaultOsToken.sol#L149

_checkOsTokenPosition(msg.sender) this is calling

  function _checkOsTokenPosition(address user) internal view {
    // fetch user position
    OsTokenPosition memory position = _positions[user];
    if (position.shares == 0) return;

    // check whether vault assets are up to date
    _checkHarvested();

    // sync fee
    _syncPositionFee(position);

    // calculate and validate position LTV
    if (
      Math.mulDiv(convertToAssets(_balances[user]), _osTokenConfig.ltvPercent(), _maxPercent) <
      _osToken.convertToAssets(position.shares)
    ) {
      revert Errors.LowLtv();
    }
  }

this is calling

  function _syncPositionFee(OsTokenPosition memory position) private view {
    // fetch current cumulative fee per share
    uint256 cumulativeFeePerShare = _osToken.cumulativeFeePerShare();

    // check whether fee is already up to date
    if (cumulativeFeePerShare == position.cumulativeFeePerShare) return;

    // add treasury fee to the position
    position.shares = SafeCast.toUint128(
      Math.mulDiv(position.shares, cumulativeFeePerShare, position.cumulativeFeePerShare)
    );
    position.cumulativeFeePerShare = SafeCast.toUint128(cumulativeFeePerShare);
  }

but note we are using memory

    OsTokenPosition memory position = _positions[user];

we does not use storage and we does not call

_positions[user] = position

after calling _syncPositionFee

so the position is never updated

Attachments

1. Proof of Concept (PoC) File

described above, this leads to accounting error and failed to update the position state of individual correctly

2. Revised Code File (Optional)

add

_positions[user] = position

after sync position

[tsudmi]

We need to use memory, because syncPositionFee is used in osTokenPositions that is view and used to get your position latest state

[JeffCX]

We need to use memory, because syncPositionFee is used in osTokenPositions that is view and used to get your position latest state

If we take a look at the usage of _syncPositionFee

it is used in mintOSToken, this line of code

then the position is updated in last logic in mintOSToken to ensure the fee is updated in this line of code

it is used in burnOSToken in this line of code

then the position is updated in the last logic in mingOSToken to ensure the fee is updated in this line of code

even the function _syncPosition is used in the view function,

it is used in the function _checkOsTokenPosition as well in this line of code

but this line of code is never called

 _positions[msg.sender] = position;

so the sync Position fee does not really sync the position fee