Rugproof Security Audit — VulnerableVault.sol

Generated 2026-05-13 · commit HEAD · chain: local

Executive summary

VulnerableVault is a 56-line ETH custody contract with a classic, fund-draining reentrancy in withdraw() that an attacker can exploit in a single transaction to drain the entire vault balance. Two privileged functions lack access control entirely — anyone can pause the contract or wipe its balance. The contract additionally ignores a low-level call return value and uses an EOA single-key admin model with sweep authority over user deposits. This contract is not safe to deploy under any circumstance.

MetricValue
Critical1
High3
Medium1
Low1
Info0
GradeF

Scope

Methodology

Trust report

The following privileged functions exist. Each represents a trust assumption.

Findings

[REENT-001 | Critical] Reentrancy drains vault via CEI violation

Confidence: High

Pattern: reentrancy (classic)

Location: examples/VulnerableVault.sol:24

Summary

withdraw() sends ether via low-level call before zeroing the user's balance. An attacker contract whose receive() re-enters withdraw() reads the un-zeroed balance, withdraws again, and repeats until the vault is empty. The bundled exploit in test/exploits/ExploitREENT001.t.sol deposits 1 ETH and walks away with 11 ETH (its deposit + the victim's 10 ETH).

Code


function withdraw() external {
    uint256 amt = balance[msg.sender];
    require(amt > 0, "no balance");

    (bool ok,) = msg.sender.call{value: amt}("");   // ← effects pending
    require(ok, "send failed");

    balance[msg.sender] = 0;       // ← state update AFTER external call
}

Impact

Loss of 100% of user deposits, in a single transaction, with no preconditions beyond depositing ≥ 1 wei.

Likelihood

Trivially high. Public exploit demonstrated; ~250K gas to execute.

Recommendation

Apply Checks-Effects-Interactions:


 function withdraw() external {
     uint256 amt = balance[msg.sender];
     require(amt > 0, "no balance");
+    balance[msg.sender] = 0;
     (bool ok,) = msg.sender.call{value: amt}("");
     require(ok, "send failed");
-    balance[msg.sender] = 0;
 }

Or add nonReentrant from OpenZeppelin's ReentrancyGuard. Prefer the CEI rewrite — it's lower gas and removes the need for the lock.

References

[ACCESS-001 | High] setPaused has no access control

Confidence: High

Pattern: access-control

Location: examples/VulnerableVault.sol:34

Summary

setPaused(bool) is external with no modifier. Any address can flip the pause state.

Code


function setPaused(bool p) external {
    paused = p;
}

Impact

Griefing — adversary can pause withdrawals indefinitely. Funds are stuck while paused (combined with the missing pause-aware code in withdraw, the situation is actually worse: pause is recorded but never checked, so this is purely state pollution today; if withdraw ever gains a whenNotPaused modifier in the future, this becomes a permanent DoS).

Recommendation


- function setPaused(bool p) external {
+ function setPaused(bool p) external onlyOwner {
     paused = p;
 }

Plus inherit Ownable (or use the existing owner field).

References

[ACCESS-002 | High] rescue can drain user deposits

Confidence: High

Pattern: access-control + centralization-risk

Location: examples/VulnerableVault.sol:40

Summary

rescue(address payable to) lets owner transfer the contract's entire ether balance — which is user deposits — to any address. There is no token allowlist, no timelock, no per-call cap, and no two-step. A single-key compromise = 100% loss.

Code


function rescue(address payable to) external {
    require(msg.sender == owner, "not owner");
    to.transfer(address(this).balance);
}

Impact

A compromised owner key drains all user deposits. Users have no recourse.

Recommendation

References

[UNCHK-001 | High] Low-level call return value ignored

Confidence: High

Pattern: unchecked-calls

Location: examples/VulnerableVault.sol:46

Summary

notifyDepositor(address depositor, bytes calldata data) invokes depositor.call(data) and discards the return value. Callers cannot tell whether the notification actually succeeded.

Code


function notifyDepositor(address depositor, bytes calldata data) external {
    depositor.call(data);          // ← return value not checked
}

Impact

Best-effort hooks become silent failures. If callers off-chain observe successful tx-level execution, they'll wrongly assume the notification reached the depositor. Gas can also be siphoned via this best-effort path (calling expensive contracts at no risk to the caller).

Recommendation


- depositor.call(data);
+ (bool ok,) = depositor.call(data);
+ require(ok, "notify failed");

Or, if best-effort is intentional, return a status code rather than silently succeeding.

References

[CENT-001 | Medium] Single-EOA owner with no two-step transfer

Confidence: High

Pattern: centralization-risk

Location: examples/VulnerableVault.sol:50

Summary

transferOwnership(address newOwner) sets owner = newOwner immediately. If the admin transfers to a wrong/unowned address (typo, hardware wallet bug), the contract is permanently bricked under a key the admin can't access.

Recommendation

Use OZ Ownable2Step:


contract VulnerableVault is Ownable2Step {
    constructor() Ownable(msg.sender) {}
    // ... rest of contract
}

This requires the new owner to call acceptOwnership() to confirm.

References

[PRAGMA-001 | Low] Floating pragma ^0.8.20

Confidence: High

Pattern: pragma-and-addresses

Location: examples/VulnerableVault.sol:2

Summary

pragma solidity ^0.8.20; floats to any 0.8.x patch release. Production deployments should pin the exact compiler used during audit.

Recommendation

pragma solidity 0.8.24;

Out of scope / limitations

Appendix

Generated by Rugproof — https://omermaksutii.github.io/RugProof