safdie - Withdrawal logic lacks sufficient validations will lead to manipulate or bypass withdrawal mechanisms in `AccountFacetImpl.sol`

[sherlock-admin4]

safdie

High

Withdrawal logic lacks sufficient validations will lead to manipulate or bypass withdrawal mechanisms in AccountFacetImpl.sol

Summary

Withdrawal logic lacks sufficient validations in AccountFacetImpl.sol will lead to manipulate or bypass withdrawal mechanisms.

Root Cause

The withdraw function in AccountFacetImpl contract has weak access control and lacks adequate validation for the amount parameter. This allows malicious actors to manipulate or bypass withdrawal mechanisms, potentially causing unauthorized withdrawals or overflows. https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/Account/AccountFacetImpl.sol#L26-L36 Key Issues:

1. Access Control: The withdraw function allows the user to withdraw funds without robust ownership checks.
2. Lack of Amount Validation: There is no explicit check that ensures the user has enough balance to cover the withdrawal request.
3. Reentrancy Attack: The function transfers funds before updating the user's balance, making it vulnerable to reentrancy attacks.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Here’s how an attacker could exploit the vulnerability to withdraw more than they are entitled to (PoC below):

1. The attacker deploys the ReentrantAttack contract and funds it with some collateral by calling the deposit function.
2. The attacker calls the withdraw function and triggers a reentrant call using the fallback function. This occurs because the external call (safeTransfer) happens before the balance is updated. The reentrancy allows the attacker to call withdraw again before the first withdrawal updates the balance, thus draining more funds than allowed.
3. The attacker successfully drains funds from the contract.

Impact

1. A malicious user could exploit the vulnerability to withdraw more funds than they deposited, leading to significant financial losses for the contract and legitimate users.
2. Since the contract interacts with multiple users' balances, exploiting this vulnerability could result in the depletion of the contract's funds, affecting all users.
3. The vulnerability allows reentrant calls to drain more than the attacker’s actual balance, making this a critical issue that could drain all available funds.

PoC

Exploit scenario: reentrancy attack on withdrawal

// Malicious Contract
contract ReentrantAttack {
    AccountFacetImpl public targetContract;
    uint256 public reentrantCalls = 0;

    constructor(address _target) {
        targetContract = AccountFacetImpl(_target);
    }

    // Fallback function for reentrancy exploit
    fallback() external payable {
        if (reentrantCalls < 3) {
            reentrantCalls++;
            targetContract.withdraw(address(this), 10 ether); // Trigger reentrancy multiple times
        }
    }

    function attack() external {
        targetContract.deposit(address(this), 10 ether); // Deposit some amount
        targetContract.withdraw(address(this), 10 ether); // Trigger withdrawal and reentrancy
    }
}

Mitigation

1. Move all external calls (safeTransfer) after updating the user’s balance to prevent reentrancy attacks.
2. Implement a reentrancy guard using a modifier to prevent reentrant calls.

3. Add a check to ensure the user has enough balance before allowing them to withdraw.

   function withdraw(address user, uint256 amount) internal nonReentrant {
   AccountStorage.Layout storage accountLayout = AccountStorage.layout();
   GlobalAppStorage.Layout storage appLayout = GlobalAppStorage.layout();
   
   uint256 amountWith18Decimals = (amount * 1e18) / (10 ** IERC20Metadata(appLayout.collateral).decimals());
   require(accountLayout.balances[msg.sender] >= amountWith18Decimals, "Insufficient balance");
   
   accountLayout.balances[msg.sender] -= amountWith18Decimals;
   
   // External call after state change to prevent reentrancy
   IERC20(appLayout.collateral).safeTransfer(user, amount);
   }