Vulnerability Report: Frozen Funds Due to Incorrect transferFrom Usage in Withdrawal Function

gistfile1.txt

Vulnerability Report: Frozen Funds Due to Incorrect transferFrom Usage in Withdrawal Function

Executive Summary

The NewtonPersonalVault contract contains a critical flaw in its _withdraw function that renders all post-expiration withdrawals impossible. The function incorrectly uses transferFrom instead of transfer when moving tokens from the vault to the beneficiary, requiring a self-allowance that is never set and cannot be set after expiration.

Vulnerability Details

Location

File: NewtonPersonalVault.sol
Function: _withdraw
Line: 164 (vulnerable code)

Vulnerable Code

solidity


function _withdraw(
    VaultTransfer calldata _transfer
) internal {
    require(
        _transfer.token.transferFrom(address(this), beneficiary, _transfer.amount),
        TransferFailed()
    );

    emit UserWithdrew(_transfer.token, _transfer.amount);
}
Root Cause Analysis

The vulnerability stems from a fundamental misunderstanding of ERC20 transfer mechanics:

transferFrom semantics: Under the ERC20 standard, transferFrom(from, to, amount) requires that allowance[from][msg.sender] >= amount. The function checks whether the caller (msg.sender) has been approved by the from address to spend tokens.
In this context:
from = address(this) (the vault)
msg.sender = address(this) (the vault, since _withdraw is called internally)
Required: allowance[vault][vault] >= amount
The missing approval: The vault never grants itself an allowance. There is no approve(address(this), ...) call anywhere in the contract.
Post-expiration lockout: The only mechanism to execute arbitrary calls from the vault is executeTransaction:
solidity


function executeTransaction(
    NewtonMessage.Attestation memory _attestation
) external vaultActive {
    // ...
}
This function is guarded by vaultActive:

solidity


modifier vaultActive() {
    require(!expired, VaultInactive());
    _;
}
Once expired = true, no further transactions can be executed through this pathway, making it impossible to set the required self-allowance.

Impact Assessment

Severity: CRITICAL

Factor	Assessment	Reasoning
Impact	High	Complete loss of access to deposited funds; core functionality broken
Likelihood	High	Occurs under normal, intended usage (deposit → operate → expire → withdraw)
Exploitability	Trivial	No special conditions required; happens automatically
Affected Functionality

Primary: withdraw() function is completely non-functional post-expiration
Secondary: All deposited ERC20 tokens become permanently frozen
Exploitation Scenarios

Scenario 1: Normal Lifecycle Leads to Frozen Funds

Context: A beneficiary uses the vault as intended.

Steps:

Beneficiary calls deposit() to deposit ERC20 tokens into the vault
Orchestrators execute various transactions via executeTransaction()
When operations conclude, an authorized party calls executeTransaction() with an attestation that triggers endOrchestratorManagement()
expired is set to true
Beneficiary calls withdraw() to retrieve their tokens
_withdraw() executes token.transferFrom(address(this), beneficiary, amount)
Transaction reverts: allowance[vault][vault] = 0
Result: Funds are permanently frozen. The beneficiary has no recourse.

Preconditions:

Standard ERC20 token behavior
Vault holds deposited balances
No pre-expiration self-allowance was configured
Scenario 2: Operational Misordering

Context: Operators intend to prepare the vault for withdrawal but execute steps in the wrong order.

Steps:

Operators plan to: (a) set self-allowances, then (b) expire the vault
Due to timing, ordering, or oversight, endOrchestratorManagement() is called first
expired = true disables executeTransaction()
Operators can no longer execute the planned approval transactions
withdraw() calls fail due to missing allowances
Result: Funds frozen due to operational error with no recovery mechanism.

Preconditions:

Misordering of expiration and approval setup
No fail-safe or recovery function exists
Scenario 3: Malicious or Compromised Orchestrator

Context: An allowlisted orchestrator acts maliciously or is compromised.

Steps:

Beneficiary deposits tokens into the vault
A malicious orchestrator has policy authorization to call endOrchestratorManagement()
Before any self-allowances are set, the orchestrator triggers expiration
Beneficiary attempts withdrawal but all calls revert
Funds remain locked indefinitely
Result: Malicious actor can grief the beneficiary by permanently freezing their funds.

Preconditions:

Malicious actor is in the orchestrator allowlist
Policy permits the orchestrator to trigger expiration
No self-allowances were pre-configured
Technical Deep Dive

Why transferFrom Fails

Consider the ERC20 transferFrom implementation pattern:

solidity


function transferFrom(address from, address to, uint256 amount) public returns (bool) {
    uint256 currentAllowance = _allowances[from][msg.sender];
    require(currentAllowance >= amount, "ERC20: insufficient allowance");
    _allowances[from][msg.sender] = currentAllowance - amount;
    _transfer(from, to, amount);
    return true;
}
When _withdraw calls token.transferFrom(address(this), beneficiary, amount):

from = vault address
msg.sender = vault address (internal call)
The token checks _allowances[vault][vault]
This value is 0 (never set)
Transaction reverts
Why transfer Works

The fix uses transfer instead:

solidity


function transfer(address to, uint256 amount) public returns (bool) {
    _transfer(msg.sender, to, amount);
    return true;
}
When the vault calls token.transfer(beneficiary, amount):

msg.sender = vault address
Tokens move from msg.sender (vault) to beneficiary
No allowance check required
Transaction succeeds
Remediation

Recommended Fix

Replace transferFrom with transfer in the _withdraw function:

solidity


function _withdraw(
    VaultTransfer calldata _transfer
) internal {
    require(
        _transfer.token.transfer(beneficiary, _transfer.amount),
        TransferFailed()
    );

    emit UserWithdrew(_transfer.token, _transfer.amount);
}
Why This Fix Works

Correct semantics: When a contract wants to send its own tokens, it should use transfer, not transferFrom
No allowance required: transfer moves tokens from msg.sender (the vault) without checking allowances
Minimal change: Single-line fix with no architectural changes required
Additional Considerations

SafeERC20 Usage: Consider using OpenZeppelin's SafeERC20 library for safer token interactions:

solidity


import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

// In contract:
using SafeERC20 for IERC20;

function _withdraw(VaultTransfer calldata _transfer) internal {
    _transfer.token.safeTransfer(beneficiary, _transfer.amount);
    emit UserWithdrew(_transfer.token, _transfer.amount);
}
This handles:

Non-standard ERC20 tokens that don't return a boolean
Tokens that revert on failure instead of returning false
Verification Steps

To confirm the vulnerability:

Deploy NewtonPersonalVault with valid modules and orchestrators
Deposit any standard ERC20 token as the beneficiary
Execute endOrchestratorManagement() via an authorized attestation
Attempt to call withdraw() as the beneficiary
Observe the transaction revert with an allowance-related error
To verify the fix:

Apply the fix (replace transferFrom with transfer)
Repeat steps 1-4 above
Confirm withdrawal succeeds and tokens reach the beneficiary
Open Questions for Review

Deposit function consistency: The _deposit function uses transferFrom(beneficiary, address(this), amount), which is correct for pulling tokens from an external address. However, it requires the beneficiary to have pre-approved the vault. Is this the intended design, or should deposits use a pull-pattern initiated externally?
Non-standard ERC20 handling: Some tokens (e.g., USDT) don't return a boolean from transfer/transferFrom. Should the contract use SafeERC20 throughout for broader compatibility?
Recovery mechanism: Given the critical nature of this vault holding user funds, should there be an emergency recovery mechanism that bypasses the vaultActive modifier, perhaps with additional safeguards (timelock, multisig)?
Pre-expiration validation: Should the contract validate that all necessary conditions for successful withdrawal are met before allowing endOrchestratorManagement() to execute?
Conclusion

This vulnerability represents a complete failure of the core withdrawal functionality. The misuse of transferFrom instead of transfer creates an impossible-to-satisfy precondition after vault expiration, permanently freezing all deposited funds. The fix is straightforward—a single function call change—but the implications of the unfixed code are severe: total and irrecoverable loss of access to deposited assets for all vault users.