vulnerable.sol
Model: claude-sonnet-4-6 | 4/27/2026
Summary
Priority fixes:
- Fix reentrancy in withdraw() by applying Checks-Effects-Interactions pattern and adding ReentrancyGuard: set balances[msg.sender] = 0 BEFORE the external call
- Add access control (onlyOwner modifier using msg.sender) to drain() and replace tx.origin with msg.sender in transferOwnership()
- Replace all transfer() calls with call() and add a MAX_BATCH_SIZE cap plus access control to batchTransfer() to prevent gas-limit DoS
critical (2)
withdraw()
The withdraw() function sends ETH to msg.sender via a low-level call BEFORE updating the balance to zero. An attacker can deploy a malicious contract whose fallback function recursively calls withdraw() again before balances[msg.sender] is set to 0, draining the entire contract balance.
Fix: Follow the Checks-Effects-Interactions pattern: update state (set balance to 0) before making any external calls. Alternatively, use OpenZeppelin's ReentrancyGuard modifier.
Show suggested fix
function withdraw() public nonReentrant {
uint256 amount = balances[msg.sender];
require(amount > 0, "No balance to withdraw");
balances[msg.sender] = 0; // Effect before interaction
(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed");
}drain()
The drain() function transfers the entire contract balance to the owner address but has no access control modifier. Any external account or contract can call this function at any time, allowing anyone to trigger a full drain of deposited funds to the owner. While funds go to owner, this disrupts user deposits unexpectedly and can be abused in combination with other exploits.
Fix: Add an onlyOwner modifier or require(msg.sender == owner) check to restrict this function to the contract owner only.
Show suggested fix
modifier onlyOwner() {
require(msg.sender == owner, "Not owner");
_;
}
function drain() public onlyOwner {
payable(owner).transfer(address(this).balance);
}high (2)
transferOwnership()
Using tx.origin for authorization is dangerous. If the owner is tricked into calling a malicious intermediary contract, that contract can call transferOwnership() and tx.origin will still be the original owner. This enables phishing attacks where ownership can be stolen without the owner's knowledge.
Fix: Replace tx.origin with msg.sender for all authentication checks.
Show suggested fix
function transferOwnership(address newOwner) public {
require(msg.sender == owner, "Not owner");
require(newOwner != address(0), "New owner is zero address");
owner = newOwner;
}batchTransfer()
The batchTransfer() function iterates over an arbitrary-length array. If the array is large enough, the transaction will exceed the block gas limit and revert, causing a denial of service. Additionally, transfer() forwards only 2300 gas, which may fail for recipient contracts with non-trivial fallback logic, and it reverts the entire batch on a single failure.
Fix: 1) Add a maximum recipients limit. 2) Use call() instead of transfer() for flexibility. 3) Implement a pull-payment pattern. 4) Add access control to prevent arbitrary callers from draining funds.
Show suggested fix
uint256 public constant MAX_BATCH_SIZE = 100;
function batchTransfer(address[] memory recipients, uint256 amount) public onlyOwner {
require(recipients.length <= MAX_BATCH_SIZE, "Batch too large");
require(address(this).balance >= amount * recipients.length, "Insufficient balance");
for (uint256 i = 0; i < recipients.length; i++) {
require(recipients[i] != address(0), "Invalid recipient");
(bool success, ) = payable(recipients[i]).call{value: amount}("");
require(success, "Transfer failed");
}
}medium (2)
constructor(), transferOwnership()
The constructor sets owner to msg.sender which is safe, but transferOwnership() does not validate that newOwner is not the zero address. If ownership is accidentally transferred to address(0), all owner-restricted functions become permanently inaccessible.
Fix: Add a require(newOwner != address(0)) check in transferOwnership().
Show suggested fix
function transferOwnership(address newOwner) public {
require(msg.sender == owner, "Not owner");
require(newOwner != address(0), "New owner cannot be zero address");
owner = newOwner;
}drain(), batchTransfer()
transfer() forwards a hardcoded 2300 gas stipend which can cause failures if the recipient is a contract with a non-trivial fallback function. Since EIP-1884 increased gas costs for certain opcodes, 2300 gas may be insufficient for even simple operations. The recommended pattern is to use call() with a success check.
Fix: Replace transfer() with call() and properly handle the return value.
Show suggested fix
function drain() public onlyOwner {
uint256 balance = address(this).balance;
(bool success, ) = payable(owner).call{value: balance}("");
require(success, "Drain failed");
}low (2)
transferOwnership(), deposit(), withdraw(), drain()
Critical state changes such as ownership transfer, deposits, and withdrawals do not emit events. This makes off-chain monitoring, debugging, and auditing significantly more difficult.
Fix: Add and emit events for all critical state changes.
Show suggested fix
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
event Deposited(address indexed user, uint256 amount);
event Withdrawn(address indexed user, uint256 amount);
event Drained(address indexed owner, uint256 amount);
function deposit() public payable {
balances[msg.sender] += msg.value;
emit Deposited(msg.sender, msg.value);
}
function withdraw() public nonReentrant {
uint256 amount = balances[msg.sender];
require(amount > 0, "No balance");
balances[msg.sender] = 0;
(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed");
emit Withdrawn(msg.sender, amount);
}
function transferOwnership(address newOwner) public {
require(msg.sender == owner, "Not owner");
require(newOwner != address(0), "Zero address");
emit OwnershipTransferred(owner, newOwner);
owner = newOwner;
}withdraw()
The withdraw() function does not check whether the caller has a non-zero balance before proceeding. This wastes gas on a zero-value external call and emits misleading state transitions.
Fix: Add require(amount > 0) before proceeding with the withdrawal.
Show suggested fix
function withdraw() public nonReentrant {
uint256 amount = balances[msg.sender];
require(amount > 0, "No balance to withdraw");
balances[msg.sender] = 0;
(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed");
}info (1)
VulnerableVault (contract-level)
The contract reimplements ownership logic and is missing reentrancy protection. OpenZeppelin provides well-audited implementations of Ownable and ReentrancyGuard that reduce the risk of custom implementation errors.
Fix: Import and inherit from OpenZeppelin's Ownable and ReentrancyGuard contracts instead of writing custom ownership and reentrancy logic.