YakuzaKiawe - Incorrect calculation in `convertToAssets`

[sherlock-admin]

YakuzaKiawe

medium

Incorrect calculation in convertToAssets

YakuzaKiawe

Medium

Incorrect calculation in convertToAssets

Summary

The calculaton for convertToAssets can get a wrong value

Vulnerability Detail

The convertToAssets function in the perennial-v2\packages\perennial-vault\contracts\Vault.sol contract has an incorrect calculation. This can lead to users having wrong shares in their account. The convertToAssets function converts a given amount of shares to assets. The calculation is as follows:

(UFixed6 _totalAssets, UFixed6 _totalShares) = (UFixed6Lib.from(totalAssets().max(Fixed6Lib.ZERO)), totalShares());
return _totalShares.isZero() ? shares : shares.muldiv(_totalAssets, _totalShares);

The problem is with the _totalShares.isZero() check. This check should be _totalAssets.isZero().

If _totalShares is zero, then the user has no shares. In this case, the correct calculation is to return the number of shares that were passed in. However, the current calculation returns the total assets in the contract, which is wrong.

If _totalAssets is zero, then the contract has no assets. In this case, the correct calculation is to return zero. However, the current calculation returns the number of shares that were passed in, which is also wrong.

The correct calculation should be as follows:

(UFixed6 _totalAssets, UFixed6 _totalShares) = (UFixed6Lib.from(totalAssets().max(Fixed6Lib.ZERO)), totalShares());
return _totalAssets.isZero() ? 0 : shares.muldiv(_totalAssets, _totalShares);

This ensures that the correct amount of assets is returned, regardless of whether the total shares or total assets in the contract are zero.

Proof of Concept

Here is an example of how the incorrect calculation can lead to users having wrong shares in their account:

1. Suppose the total assets in the contract are 100.
2. Suppose the user has 50 shares.
3. The user calls the convertToAssets function with 25 shares.
4. The incorrect calculation will return 25, which is the number of shares that were passed in.
5. However, the correct calculation would return 12.5, which is the amount of assets that the user should have received.

This means that the user would have 25 shares in their account, but they would only have 12.5 assets.

Impact

The incorrect calculation in convertToAssets could lead to users having wrong shares in their account. This could result in users losing money or being unable to withdraw their assets.

Code Snippet

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

Tool used

Manual Review

Recommendation

Correct the calculation by doing this:

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

This will ensure that the correct amount of assets is returned, regardless of whether the total shares or total assets in the contract are zero.

[sherlock-admin]

5 comment(s) were left on this issue during the judging contest.

141345 commented:

l

n33k commented:

invalid

0xyPhilic commented:

invalid because if totalShares is 0 this means also totalAssets is 0

darkart commented:

Invalid because its breaking Sherlock rules on annonymous judging !!!

panprog commented:

invalid because this is a view function not used anywhere which is invalid according to sherlock rules