sharesToBurn in _redeemOsToken should round up but didn't

[hats-bug-reporter[bot]]

Github username: @turvec Submission hash (on-chain): 0x392287e70d2ea5e3d056d33b89cef08d00f2c6d6b270fc5c766c401f0394d11d Severity: low

Description: Description\ sharesToBurn in _redeemOsToken should round up but didn't which favor the users and not the vault itself in both redeemOsToken and liquidateOsToken calls, causing VaultOsToken.sol to not be ERC4626-compliant

Attack Scenario\

• Bob calls redeemOsToken
• this internally attempts to burn bob shares
• the amount of Bob shares to be burnt is rounded down in favor of bob and not the vault itself

Attachments

1. Proof of Concept (PoC) File

according to EIP-4626- Vault, implementers should be aware of the need for specific, 𝐨𝐩𝐩𝐨𝐬𝐢𝐧𝐠 𝐫𝐨𝐮𝐧𝐝𝐢𝐧𝐠 𝐝𝐢𝐫𝐞𝐜𝐭𝐢𝐨𝐧𝐬 across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

• If it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should 𝐫𝐨𝐮𝐧𝐝 𝐝𝐨𝐰𝐧.

• If it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should 𝐫𝐨𝐮𝐧𝐝 𝐮𝐩.

https://eips.ethereum.org/EIPS/eip-4626 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L229

However in the implementation of _redeemOsToken() which is called in both redeemOsToken and liquidateOsToken.

    uint256 sharesToBurn = convertToShares(receivedAssets);

the function calculates the user shares to burn but instead of using the internal _convertToShares, it call the public convertToShares() which Rounds Down by default

   function convertToShares(uint256 assets) public view override returns (uint256 shares) {
    return _convertToShares(assets, Math.Rounding.Down);
  }

and then burn user shares using the Rounded Down value

     _burnShares(owner, sharesToBurn);

2. Revised Code File (Optional)

An easy fix is to change this line of code:

    uint256 sharesToBurn = convertToShares(receivedAssets);

to this:

    uint256 sharesToBurn = _convertToShares(receivedAssets, Math.Rounding.Up);

[tsudmi]

We're not following the ERC-4626 standard here, except we use some of the function names from there.

[turvec]

Hi @tsudmi I agree, There are no strict requirements to be EIP-4626 compliant hence the low. However, it's in terms of liquidity tokens and Vault always takes a bit less. ensuring it remains secure and doesn't run out of tokens is what the revised code provides.