SanketKogekar - Scope for reentrancy in `withdraw()` function of `FootiumAcademy` and `FootiumEscrow`

[sherlock-admin]

SanketKogekar

medium

Scope for reentrancy in withdraw() function of FootiumAcademy and FootiumEscrow

Summary

Lack of nonReentrant in withdraw functions makes it possible for attackers to break functionality using attack contracts.

Also one other is that function completes the execution and never reverts even if the user's withdrawable balance is 0. This makes it consume gas for no specific reason.

Vulnerability Detail

One other issue of withdraw() function is that it completes the execution and never reverts even if the user's withdrawable balance is 0. This makes it consume gas for no specific reason.

Consider the function:

function withdraw() external onlyClubOwner {
        uint256 balance = address(this).balance;
        if (balance > 0) {
            (bool sent, ) = payable(msg.sender).call{value: balance}("");
            if (!sent) {
                revert FailedToSendETH(balance);
            }
        }
    }

Here, the function completes execution even if balance is 0.

Impact

Code Snippet

https://github.com/sherlock-audit/2023-04-footium/blob/main/footium-eth-shareable/contracts/FootiumAcademy.sol#L218

https://github.com/sherlock-audit/2023-04-footium/blob/main/footium-eth-shareable/contracts/FootiumAcademy.sol#L218

Tool used

Manual Review

Recommendation

Consider adding nonReentrant or ReentrancyGuard to make it robust, and add following line to revert if user balance is 0 :

require(balance > 0, "Not enough balance")