Mystery Box

Summary

The 'withdrawFunds' function is vulnerable to reentrancy attacks. It’s crucial to follow the “checks-effects-interactions” pattern to prevent this.

Vulnerability Details

The withdrawFunds() function transfers the entire contract balance to the owner using a low-level call: call{value: address(this).balance}("").
After the call is made, the control is transferred to the owner, which could be a malicious contract. The malicious contract's fallback function could then immediately call the withdrawFunds() function again before the first execution is completed.
Since the contract's balance is not updated until after the external call, the malicious contract can repeatedly call withdrawFunds() and withdraw the balance multiple times, draining the contract.

Findings

https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L67C1-L71C6

Impact

Since the contract's balance is not updated until after the external call, the malicious contract can repeatedly call withdrawFunds() and withdraw the balance multiple times, draining the contract.

Tools Used

Manual analysis

Recommendations

you could use a pull-payment approach, where users request a withdrawal and then manually withdraw their funds later. This pattern further decouples the external call from state-changing logic.