Loss of funds due to decimal precision inconsistency

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x9b1ed0ed45bed1ae3b907387a891ae2aa41eb7d790a80c976821c11911a4e7c6
Severity: medium

Description:
Description
The contract implements the assetsToShares functions which is called by the convertToShares function to calculate the amount of shares for an amount of input tokens. The function convertToShares is called during the execution of the deposit function. This function allows the user to burn an amount of tokens in exchange for some shares.

However, due to the calculation of the share amount in the assetsToShares function, there is a non-zero amount of tokens that can be deposited by the user and in exchange yield zero shares. Specifically, given the expression

Math.mulDiv(assets, 10 ** 18, currentPrice);

if assets is less than 10 ** 18, the amount of shares calculated for the user is zero. As a result, the user will proceed burning their tokens while they receive no shares in exchange. The contract should protect the users from loss of funds to ensure consistency and trust.

1. Proof of Concept (PoC) File

The contract does not prevent users from depositing less than 10 ** 18 tokens and at the same time it does not ensure that the lowest possible deposit receives at least 1 share. Therefore, deposited amounts of this scale are irreversibly lost.

Recommendation

The team is advised to resolve this issue by either preventing users from depositing less than 10 ** 18 tokens or by increasing the share basis by a factor of 10 ** 18 so that 1Wei of deposited tokens is entitled 1 share.

[0xupbill]

I would like to update the description to explicitely state that all deposited assets < currentPrice/10 ** 18 receive no shares while accepted as valid deposits and the respective tokens are burned.

[AndreiMVP]

I don't see the issue here. OZ Math library handles the rounding. Is it about a loss due to the exact precision like #33? because it wouldn't be a prob in practice

[0xupbill]

It is the same issue from the side of the deposit. Imo since it allows the loss of value for a user it should be classified as a bug even if it is of moderate significance. The same is true for #33. both issues cn be resolved by increasing the base to account for the smallest units

[AndreiMVP]

I get losing a v small unit isn't perfect but it's not something I would worry about in practice since we're dealing with >= $1 stablecoins. So I don't see this as an issue.