code-423n4 / 2023-08-dopex-findings

3 stars 3 forks source link

Upgraded Q -> 2 from #2037 [1698131784473] #2230

Closed c4-judge closed 10 months ago

c4-judge commented 10 months ago

Judge has assessed an item in Issue #2037 as 2 risk. The relevant finding follows:

[L‑01] Early users can modify the underlying assets’ unit share price Summary:

ERC4626, an extension of ERC20, is a standard that is mostly used in yield-bearing tokens. The contract of an ERC4626 token itself is an ERC20 which serves as a “shares” token and has an underlying asset which is another ERC20. The idea behind this is in such a way that the users deposit their assets and get corresponding shares. The function convertToShares() is responsible for returning the corresponding share of a user with respect to his deposited assets.

function convertToShares(
uint256 assets

) public view returns (uint256 shares) { uint256 supply = totalSupply; uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

uint256 totalVaultCollateral = totalCollateral() +
  ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
return
  supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);

} One of the main problems with these tokens lies in the fact that the first depositor can manipulate the unit share price in a bad way. The ratio of $\large\dfrac{totalSupply}{totalAssets}$ determines the corresponding share of a deposited asset. As it is clear from the ratio if the first user deposits 1 Wei, will get 1 Wei worth of share. Now consider a scenario with the preceding steps:

1 - The same bad guy deposits $\large 10000 \times 10^{18} - 1$ amount of underlying assets. 2 - As a consequence, the abovementioned ratio skyrockets and won’t be a 1:1 pair anymore. $\large \dfrac{10000 \times 10^{18} - 1 + 1}{1} = 10^{22}$ 3 - The later users who deposit large amounts of assets (e.g. $\large 19999 \times 10^{18} - 1 $) will get 1 Wei worth of shares. 4 - Thus, if they call redeem() after depositing, they will immediately lose $\large 9999 \times 10^{18} - 1$ (nearly half of) the assets.

There are instances of this issue:

function convertToShares( uint256 assets ) public view returns (uint256 shares) { uint256 supply = totalSupply; uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

uint256 totalVaultCollateral = totalCollateral() +
  ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
return
  supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);

} Link(s): https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L274C1-L284C4

c4-judge commented 10 months ago

GalloDaSballo marked the issue as duplicate of #863

c4-judge commented 10 months ago

GalloDaSballo marked the issue as satisfactory