Petite Spruce Mammoth - Lack of input validation on the `amount` parameter will lead to various unintended behaviors in `AccountFacet.sol`

[sherlock-admin2]

Petite Spruce Mammoth

Low/Info

Lack of input validation on the amount parameter will lead to various unintended behaviors in AccountFacet.sol

Summary

Lack of input validation on the amount parameter in AccountFacet.sol will lead to various unintended behaviors.

Root Cause

In the AccountFacet contract, multiple functions such as deposit, withdraw, depositAndAllocate, etc., accept an amount parameter without explicitly checking if the amount is valid (e.g., greater than 0). This can lead to various unintended behaviors, including unnecessary gas consumption, inconsistent state changes, and even abuse if not checked properly. https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/Account/AccountFacet.sol#L17-L20 PoC below demonstrating how this lack of validation can be abused: Vulnerable Functions:

• deposit(uint256 amount)
• withdraw(uint256 amount)
• depositAndAllocate(uint256 amount) In this PoC, an attacker can call these functions with amount = 0, which can lead to unnecessary state changes and event emissions, increasing gas consumption or causing other unintended consequences. The attacker or any user can deploy the MaliciousUser contract, passing in the AccountFacet address. They can then call exploitDeposit or exploitWithdraw, which will invoke the vulnerable functions with amount = 0.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

With PoC below:

1. The attacker calls deposit(0) or withdraw(0).
2. The contract emits an unnecessary Deposit or Withdraw event despite no actual tokens being deposited or withdrawn.
3. This causes unnecessary gas usage and bloats the event logs, leading to higher costs for the network.

Impact

1. Users can call the deposit/withdraw functions with amount = 0, triggering unnecessary state changes and event emissions. Although no tokens are transferred, this still results in wasted gas costs for both the user and the network. Since events are emitted regardless of the amount, logs and data storage could increase unnecessarily, bloating the event logs and increasing costs for reading logs during debugging or off-chain data collection.
2. While this issue alone may not lead to a critical denial of service (DoS), it could be exploited in conjunction with other vulnerabilities or gas limits to cause unexpected behavior or minor disruptions in the contract flow.
3. Although no tokens are moved when amount = 0, emitting events like Deposit or Withdraw can create confusion in interpreting the state of the system. For example, off-chain systems or front-end interfaces relying on these events may display incorrect balances or assume a successful transaction took place.
4. Functions such as depositAndAllocate perform additional logic based on the deposit. Allowing an amount = 0 without validation could result in faulty allocation logic or other related functions proceeding based on invalid inputs.

PoC

PoC for the deposit function:

contract MaliciousUser {
    AccountFacet public accountFacet;

    constructor(address _accountFacet) {
        accountFacet = AccountFacet(_accountFacet);
    }

    function exploitDeposit() public {
        // Attacker calls the deposit function with an amount of 0
        accountFacet.deposit(0);
    }

    function exploitWithdraw() public {
        // Attacker calls the withdraw function with an amount of 0
        accountFacet.withdraw(0);
    }
}

Mitigation

To prevent this issue, add validation to ensure that the amount parameter is greater than zero before proceeding with the deposit, withdrawal, or allocation logic.

function deposit(uint256 amount) external whenNotAccountingPaused {
    require(amount > 0, "Amount must be greater than 0");
    AccountFacetImpl.deposit(msg.sender, amount);
    emit Deposit(msg.sender, msg.sender, amount);
}

function withdraw(uint256 amount) external whenNotAccountingPaused notSuspended(msg.sender) {
    require(amount > 0, "Amount must be greater than 0");
    AccountFacetImpl.withdraw(msg.sender, amount);
    emit Withdraw(msg.sender, msg.sender, amount);
}

This simple check ensures that the contract only performs valid operations with meaningful inputs, mitigating unnecessary gas consumption and preventing confusing event emissions.