sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

w42d3n - Reentrancy Vulnerabilities in TitlesCore.sol #428

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 5 months ago

w42d3n

medium

Reentrancy Vulnerabilities in TitlesCore.sol

Summary

The contract TitlesCore is susceptible to reentrancy attacks in several places. While it uses the nonReentrant modifier from OpenZeppelin, this is not applied consistently, and missing in several function calls.

Vulnerability Detail

Re-entrancy vulnerabilities occur when a function calls another function (either external or internal) in such a way as to allow new requests to the original function to be processed before the original function's processing is completed. This allows an attacker to potentially manipulate state variables and drain the contract's balance.

Impact

For example, in the publish function there's an external call to edition_.publish which can change the state of the contract. If the edition_ contract has a bug or malicious code, it could reenter into the TitlesCore.publish function and changes could be made on the unchecked state.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/TitlesCore.sol#L126-L148

// wake-disable-next-line reentrancy
tokenId = edition_.publish(
    work_.creator.target,
    work_.maxSupply,
    work_.opensAt,
    work_.closesAt,
    work_.attributions,
    work_.strategy,
    work_.metadata
);

// wake-disable-next-line reentrancy
feeManager.collectCreationFee{value: msg.value}(edition_, tokenId, msg.sender);

// wake-disable-next-line reentrancy
Target memory feeReceiver = feeManager.createRoute(
    edition_, tokenId, _attributionTargets(work_.attributions), referrer_
);

// wake-disable-next-line reentrancy
edition_.setRoyaltyTarget(tokenId, feeReceiver.target);

Each of these calls are followed by the comment wake-disable-next-line reentrancy, indicating that reentrancy checks have been deliberately disabled on the next line.

Tool used

Manual Review

Recommendation

Ensure the nonReentrant modifier is consistently used, especially when making external function calls that can affect contract state. Secondly, always take actions in the following order: 1) Decrease balance 2) Change state 3) External Calls.