Reentrancy in a Payout Queue
A Solidity contract lets users withdraw accrued rewards. The function reads the user's balance into a local variable, calls msg.sender.call{value: amount}("") to pay out, and only then sets balances[msg.sender] = 0. It returns a boolean and reverts on failure. A separate emergencyWithdraw function is guarded by onlyOwner and uses transfer. During a testnet exercise, an attacker contract's receive() function re-enters withdraw and drains funds. The team's lead argues the bug is impossible because the contract compiles cleanly and the unit tests pass. They also point out that transfer is used elsewhere, so "the pattern is already safe." A code freeze is in 48 hours.
1. Explain why the tests passed and why the `transfer` argument does not protect the vulnerable function.
Read the worked answer
The tests passed because they almost certainly called withdraw from an externally owned account. An EOA has no code, so the low-level call returns immediately without executing any fallback, and the state update happens before control returns to the test. The reentrancy window only opens when the recipient is a contract with a receive or fallback function that calls back in. Passing tests therefore say nothing about the vulnerable path; the test suite lacked a malicious-contract fixture.
The transfer argument is a category error. transfer forwards a fixed 2,300 gas stipend, which historically limited what a recipient could do, but that protection applies only to functions that use transfer. The vulnerable function uses call{value:} with all available gas, so the recipient can re-enter freely. Moreover, relying on the 2,300 stipend is itself fragile after gas repricing and breaks legitimate recipients such as multisigs and smart contract wallets. The correct fix is structural: follow checks-effects-interactions by zeroing the balance before the external call, and add a reentrancy guard as defense in depth. Neither change depends on the gas stipend.
2. The team wants the smallest safe change under time pressure. What do you recommend, and what would you verify before the freeze?
Read the worked answer
The smallest safe change is to reorder the function: check the balance, set balances[msg.sender] = 0 before the external call, then perform the call and revert if it fails. This closes the specific reentrancy path because the second entry sees a zero balance. It is a one-line move and easy to review.
I would still add a reentrancy guard modifier on withdraw and any other function that makes external calls or transfers value. Reordering fixes this bug, but guards protect against future edits and against cross-function reentrancy that a single reorder may not cover. The guard is cheap and well understood.
Before the freeze I would verify four things. First, a test with a malicious recipient contract that attempts re-entry must fail to drain funds and must show the expected revert. Second, a test where the recipient reverts must leave the balance intact, confirming the state update is rolled back. Third, confirm no other function reads the balance between the call and the update. Fourth, confirm the change does not break the emergencyWithdraw path or introduce a double-spend where a failed call leaves the balance zeroed incorrectly - the revert handles that, but it must be tested. I would also check whether a pull-payments pattern (users withdraw to a credit balance) is a better long-term design, but not block the freeze on it.