Add reentrancy protection in `payArtist` function

[c4-submissions]

Lines of code

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

Vulnerability details

In payArtist function, the potential for reentrancy exists due to the call method being used to transfer Ether. The call method is known to be risky for reentrancy attacks because it hands off control to the called contract, which could be malicious. After the Ether transfer via call, if there is more code executing that modifies the contract's state, it might be vulnerable to reentrancy.

contract Malicious {
    address payable public target;
    uint256 public drainCount = 0;

    constructor(address payable _target) {
        target = _target;
    }

    // Fallback function invoked when Ether is received
    receive() external payable {
        if (drainCount < 3) { // Prevents infinite loop, adjust based on the attack scenario
            drainCount++;
            YourContract(target).payArtist(/* parameters */);
        }
    }

    function attack() external payable {
        // Call payArtist with appropriate parameters
        YourContract(target).payArtist(/* parameters */);
    }

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

Mitigation

function payArtist(uint256 _collectionID, address _team1, address _team2, uint256 _teamperc1, uint256 _teamperc2) public FunctionAdminRequired(this.payArtist.selector) noReentrant {
    // Function logic...
}

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #2039

[c4-pre-sort]

141345 marked the issue as duplicate of #51

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[alex-ppg]

The Warden specifies that a re-entrancy can affect the payArtist function of the MinterContract, however, they have failed to pinpoint exactly how and a brief analysis of the function indicates that it is not susceptible to a re-entrancy as the royalties (collectionTotalAmount) is erased immediately after being read and before any transfer.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid