Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Certainly. Here's a report for the issue you've identified: #77

Open
hats-bug-reporter bot opened this issue Jun 30, 2024 · 1 comment
Open

Certainly. Here's a report for the issue you've identified: #77

hats-bug-reporter bot opened this issue Jun 30, 2024 · 1 comment
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

Github username: --
Twitter username: --
Submission hash (on-chain): 0x45af594e37907786efc19eea5046d4b897518936ba6dc1ee0e011e4b1bfa26dd
Severity: high

Description:
Description
In the getRedeemAssetsAndFees function of the EthMultiVault contract, there's an inconsistency in the order of fee calculations. The exit fee is calculated based on the asset amount after the protocol fee has been deducted, rather than on the full asset amount before any fees. This approach may lead to lower exit fees than expected and could potentially be exploited.

Attack Scenario
While not a direct attack vector, this inconsistency can lead to the following issues:

  1. Users redeeming large amounts of shares could pay disproportionately lower exit fees.
  2. The protocol may collect less in exit fees than intended over time.
  3. This could be exploited in combination with other mechanisms to maximize returns at the expense of the protocol or other users.

For example:

  1. A user identifies this discrepancy.
  2. They wait for a situation where the protocol fee is high.
  3. They perform a large redemption, benefiting from a lower exit fee calculated on the post-protocol fee amount.
  4. This could be repeated to consistently pay lower exit fees on large redemptions.

Attachments

uint256 assetsForReceiverAfterprotocolFee = assetsForReceiverBeforeFees - protocolFee;

  1. Revised Code File (Optional)
function getRedeemAssetsAndFees(uint256 shares, uint256 id) public view returns (uint256, uint256, uint256, uint256) {
    uint256 remainingShares = vaults[id].totalShares - shares;

    uint256 assetsForReceiverBeforeFees = convertToAssets(shares, id);
    uint256 protocolFee;
    uint256 exitFee;

    if (paused()) {
        exitFee = 0;
        protocolFee = 0;
    } else if (remainingShares == generalConfig.minShare) {
        exitFee = 0;
        protocolFee = protocolFeeAmount(assetsForReceiverBeforeFees, id);
    } else {
        // Calculate exit fee before protocol fee
        exitFee = exitFeeAmount(assetsForReceiverBeforeFees, id);
        protocolFee = protocolFeeAmount(assetsForReceiverBeforeFees, id);
    }

    uint256 totalUserAssets = assetsForReceiverBeforeFees;
    uint256 assetsForReceiver = assetsForReceiverBeforeFees - exitFee - protocolFee;

    return (totalUserAssets, assetsForReceiver, protocolFee, exitFee);
}
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Jun 30, 2024
@mihailo-maksa mihailo-maksa added the invalid This doesn't seem right label Jul 1, 2024
@mihailo-maksa
Copy link
Collaborator

The report suggests that the getRedeemAssetsAndFees function incorrectly calculates fees, leading to lower exit fees than expected.

Label: invalid

Comment:
The sequential fee charging is by design. Exit fees are not collected by the protocol but are intended to increase the share prices for remaining shareholders, benefiting them. The current implementation ensures this design.

Comment on the issue:
The sequential fee charging is by design, with exit fees intended to benefit remaining shareholders by increasing share prices. Therefore, the current implementation is valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

1 participant