code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In the code4rena dopex contests is especified the following about the PerpetualAtlanticVaultLP contract:

Contract for the Perpetual Atlantic Vault LP (ERC4626).

But the PerpetualAtlanticVaultLP.deposit/mint functions do not work well with fee-on-transfer tokens as the assets variable is the pre-fee amount, including the fee, whereas the totalAssets do not include the fee anymore.

This vulnerability can result in malicious users getting more shares than they should have gotten when doing mint/deposit

Proof of Concept

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

/**
* @notice deposit into ERC4626 token
* @param assets assets
* @param receiver receiver
* @return shares shares of LP tokens minted
*/
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);
}

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L128
```
collateral.transferFrom(msg.sender, address(this), assets);
```
The implementation can be abused to mint more shares than desired for example:

A deposit(1000) should result in the same shares as two deposits of deposit(500) but it does not because assets is the pre-fee amount.

Assume a fee-on-transfer of 20%. Assume current totalAmount = 1000, totalShares = 1000 for simplicity.

deposit(1000) = 1000 / totalAmount * totalShares = 1000 shares.

deposit(500) = 500 / totalAmount * totalShares = 500 shares. Now the totalShares increased by 500 but the totalAssets only increased by (100% - 20%) * 500 = 400. Therefore, the second deposit(500) = 500 / (totalAmount + 400) * (newTotalShares) = 500 / (1400) * 1500 = 535.714285714 shares.

In total, the two deposits lead to 35 more shares than a single deposit of the sum of the deposits.

Tools Used

Manual review.

Recommended Mitigation Steps

assets should be the amount excluding the fee, i.e., the amount the contract actually received.

This can be done by subtracting the pre-contract balance from the post-contract balance. However, this would create another issue with ERC777 tokens.

Maybe previewDeposit should be overwritten by vaults supporting fee-on-transfer tokens to predict the post-fee amount and do the shares computation on that.