Rounding Issues In convertToShares function

Summary

The mulDivoperation rounds down and creates problems in arithmetic.

Vulnerability Detail

convertToSharesis used in the Vault code to know how many shares correspond to a certain amount of assets. Essentially, the‌‌ number‌‌ of‌‌ shares‌‌ that‌‌ a‌‌ user‌‌ will‌‌ receive‌‌ depends‌‌ on‌‌ the‌‌ total‌‌ shares,‌‌ the‌‌ total‌‌ amount‌‌ of‌‌ assets,‌‌ and‌‌ the‌‌ amount‌‌ of‌‌ deposited‌‌ assets.‌‌ The‌‌ total‌‌ asset amount‌‌ is‌‌ computed‌‌ by‌‌ the‌‌ following‌‌ code:

function convertToShares(UFixed6 assets) external view returns (UFixed6) { (UFixed6 _totalAssets, UFixed6 _totalShares) = (UFixed6Lib.from(totalAssets().max(Fixed6Lib.ZERO)), totalShares()); return _totalShares.isZero() ? assets : assets.muldiv(_totalShares, _totalAssets); }

However, the current implementation of convertToSharesfunction will round down the number of shares returned due to how solidity handles Integer Division. Consequently, it will provide slightly fewer shares than expected for some amount of assets.

Impact

The function will return fewer number of shares than expected.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L113-L117

https://github.com/sherlock-audit/2023-07-perennial/blob/main/root/contracts/number/types/UFixed6.sol#L195-L197

function muldiv(UFixed6 a, UFixed6 b, UFixed6 c) internal pure returns (UFixed6) { return UFixed6.wrap(UFixed6.unwrap(a) * UFixed6.unwrap(b) / UFixed6.unwrap(c)); }

Tool used

Manual Review + in-house tool

Recommendation

Ensure that the rounding of vault’s functions behave as expected. Alternatively, if such alignment of rounding could not be achieved due to technical limitation, at the minimum, document this limitation in the comment so that the developer performing the integration is aware of this.

sherlock-audit / 2023-07-perennial-judging

hassan-truscova - Rounding Issues In convertToShares function #98