Variable used before assignation on function `beforeDeposit`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L60-L78 ttps://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L80-L97 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L278-L282

Vulnerability details

Author: rotcivegaf

Impact

When the shares/assets parameters are passed in the function beforeDeposit they are not assigned yet:

• shares in function deposit, it's assigned in L68
• assets in function mint, it's assigned in L87

In the both cases the value of the variable(shares/assets) it's 0 As the PirexERC4626 it's an abstract contract some new vault contract can inherit from him, like AutoPxGmx and AutoPxGlp If this new vault implement the function beforeDeposit using the shares or assets parameters for a calculation, the function deposit and mint could be broke

Note: The current contracts don't use the shares/assets parameters, but because it's a abstract contract this issue should be taken into consideration

Another consideration: remove the if (totalAssets() != 0), the function beforeDeposit should call in all cases and the case of totalAssets it's 0 should be resolve inside the function beforeDeposit

Proof of Concept

File: src/vaults/PirexERC4626.sol

/// @audit: `shares` defined in L68
65:        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

/// @audit: `assets` defined in L87
85:        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

Recommended Mitigation Steps

Move the beforeDeposit after the shares/assets definition

@@ -62,11 +62,11 @@ abstract contract PirexERC4626 is ERC20 {
         virtual
         returns (uint256 shares)
     {
-        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);
-
         // Check for rounding error since we round down in previewDeposit.
         require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

+        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);
+
         // Need to transfer before minting or ERC777s could reenter.
         asset.safeTransferFrom(msg.sender, address(this), assets);

@@ -82,10 +82,10 @@ abstract contract PirexERC4626 is ERC20 {
         virtual
         returns (uint256 assets)
     {
-        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);
-
         assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.

+        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);
+
         // Need to transfer before minting or ERC777s could reenter.
         asset.safeTransferFrom(msg.sender, address(this), assets);

[Picodes]

Interesting remarks but this does not lead to a finding in the context of this contest when PirexERC4626 is used

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-11-redactedcartel-findings/issues/360