Dopex - Inspecktor's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118-L135

Vulnerability details

Impact

Inflation share price can be done by depositing as soon as the vault is created.

Impact:

1. Early depositor will be able to steal other depositors funds
2. Exchange rate is inflated. As a result, depositors are not able to deposit small funds.

The problem exists because the exchange rate is calculated as the ratio between shares totalSupply and totalAssets(). When an attacker transfers assets, totalAssets() incrementally increases and hence the exchange rate also increases.

ERC4626 vaults are subject to a share price manipulation attack that allows an attacker to steal the underlying tokens from other contributors (this is a known issue with the Solmate ERC4626 implementation (https://github.com/transmissions11/solmate/issues/178)).

Proof of Concept

1. Alice - the first custodian of the PerpetualAtlanticVaultLP;

2. Alice contributes 1 wei tokens;

3. In the deposit() (https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118-L135) function, the number of shares is calculated using previewDeposit () functions: function deposit( uint256 assets, address receiver ) public virtual returns (uint256 shares) { // Check for rounding error since we round down in previewDeposit. require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

   perpetualAtlanticVault.updateFunding();

   // Need to transfer before minting or ERC777s could reenter. collateral.transferFrom(msg.sender, address(this), assets);

   _mint(receiver, shares);

   _totalCollateral += assets;

   emit Deposit(msg.sender, receiver, assets, shares); }

4. Since Alice is the first contributor (totalSupply is 0), she gets 1 share (1 wei);

5. Then Alice sends 9999999999999999999 tokens (10e18 - 1) to the vault;

6. The price for 1 share is now 10 tokens: Alice is the only depositor in the vault, she holds 1 wei of shares, and the pool balance is 10 tokens;

7. Bob contributes 19 tokens and only gets 1 share due to rounding in the convertToShares function: 10e18 * 1 / 10e18 == 1;

8. Alice redeems her share and receives half of the deposited assets, 14.5 tokens (minus the withdrawal fee);

9. Bob redeems his share and receives only 14.5 tokens (minus withdrawal fees) instead of the 19 tokens he deposited.

Tools Used

Manual review

Recommended Mitigation Steps

Consider any of these options:

1. In the deposit function, consider requiring a fairly high minimum amount of assets on your first deposit. The amount should be high enough to mint many shares to reduce rounding error, and low enough to be accessible to users.
2. When making your first deposit, consider issuing a fixed and large number of shares, regardless of the deposit amount.
3. Consider filling up pools during deployment. This must be done in deployment transactions to avoid preemptive attacks. The sum must be high enough to reduce the rounding error.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1016-L1042

Vulnerability details

Impact

The RdpxV2Core.sol contract implements the pause functionality with the ability to check whether the contract is in pause mode (function _whenNotPaused()). _whenNotPaused() is present in almost all public functions. However, the redeem() function (https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1016-L1042) does not have it.

Proof of Concept

1. An emergency occurs, such as a hack, in which a further withdrawal of funds may result in the loss of funds by users and the protocol.
2. The protocol command can suspend the execution of public functions.
3. However, the protocol command cannot suspend RdpxV2Core.redeem().
4. As a result, funds may be lost.

Tools Used

Manual review

Recommended Mitigation Steps

Add _whenNotPaused() to the redeem() function.

Assessed type

Access Control