search

sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

threadmodeling - `TitlesCore._publish()` can be DOSd by consuming the user's allowance by frontrunning `FeeManager.collectCreationFee()` #251

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 4 months ago

threadmodeling

high

TitlesCore._publish() can be DOSd by consuming the user's allowance by frontrunning FeeManager.collectCreationFee()

Summary

When using an ERC20 for fees, the PUBLISHER_ROLE's allowance given to FeeManager or the one of the user calling createEdition() can be freely consumed through FeeManager.collectCreationFee(). This will move funds to the FeeManager's protocolFeeReceiver and DOS the TitlesCore._publish() function (which is the main functionality on the TitlesCore contract).

Vulnerability Detail

In the TitlesCore._publish() function, FeeManager.collectCreationFee() is called:

File: TitlesCore.sol
120:     function _publish(Edition edition_, WorkPayload memory work_, address referrer_)
...
138:         feeManager.collectCreationFee{value: msg.value}(edition_, tokenId, msg.sender);

FeeManager.collectCreationFee() isn't permissioned or access-controlled, therefore it can be called by anyone:

File: FeeManager.sol
166:     function collectCreationFee(IEdition edition_, uint256 tokenId_, address feePayer_)
167:         external
168:         payable
169:     {
170:         Fee memory fee = getCreationFee();
171:         if (fee.amount == 0) return;
172: 
173:         _route(fee, Target({target: protocolFeeReceiver, chainId: block.chainid}), feePayer_);
174:         emit FeeCollected(address(edition_), tokenId_, ETH_ADDRESS, fee.amount, 0);
175:     }

As we can see above, if there's a fee.amount to pay, the receiver is fetched from storage but the feePayer_ depends on the (malicious) user's input.

The _route function is basically a transfer that can either occur with native Ether or with an ERC20 token:

File: FeeManager.sol
448:     function _route(Fee memory fee_, Target memory feeReceiver_, address feePayer_) internal {
449:         // Cross-chain fee routing is not supported yet
450:         if (block.chainid != feeReceiver_.chainId) revert NotRoutable();
451:         if (fee_.amount == 0) return;
452: 
453:         _transfer(fee_.asset, fee_.amount, feePayer_, feeReceiver_.target); 
454:     }
...
461:     function _transfer(address asset_, uint256 amount_, address from_, address to_) internal {
462:         if (asset_ == ETH_ADDRESS) {
463:             to_.safeTransferETH(amount_);
464:         } else {
465:             asset_.safeTransferFrom(from_, to_, amount_);
466:         }
467:     }

Given that, here, the from parameter is controlled by the malicious user's input, it means that the frontrunning attack will transfer fee.amount of fee.asset from victim to protocolFeeReceiver.

While the amount isn't user controlled: multiple calls to collectCreationFee() can be made to either drain the victim's set allowance or funds (if max approval was given), whichever gives in first.

The funds will probably be recoverable due protocolFeeReceiver being probably trusted to send them back, but draining the allowance will make any subsequent calls to TitlesCore._publish() to revert, at only the cost of gas calls for the attacker, effectively DOSing the publish functionality on TitlesCore

Impact

While DOS attacks vary in severity, this one is easy and cheap for the attacker. Also, DOSing the PUBLISHER_ROLE and the TitlesCore._publish() will additionally move their funds around. Hence, this is submitted as a High Severity bug

Code Snippet

Tool used

Manual Review

Recommendation

Seeing a transferFrom function with from being something else than msg.sender will often result in such a bug. The flow of funds needs to be changed so that FeeManager's functions become permissioned or that approval is given and reset in the same transaction.

pqseags commented 4 months ago

Is this only valid for ERC20 fees, or for ETH fees? If ERC20 then this shouldn't be in scope, but if it is valid for ETH as well then it is in scope.

goheesheng commented 3 months ago

Quoted from here https://github.com/sherlock-audit/2024-04-titles-judging/issues/69

"Only ETH is supported per the contest Q&A. Won't fix."

Hence this issue should be invalidated

0x73696d616f commented 3 months ago

Escalate

Out of scope, as the comments above imply.

sherlock-admin3 commented 3 months ago

Escalate

Out of scope, as the comments above imply.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

BiasedMerc commented 3 months ago

In discord, the protocol sponsor clearly stated:

pqseags — 24/04/2024 21:55 I wanted to clear up a common question - while the readme states that any ERC20 token is supported, actually only ETH >is fully supported in all parts of the protocol.

Therefore this is clearly invalid as it related to ERC20

sainimukul1911 commented 3 months ago

Per the contest Readme of Future Integrations " Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold? Yes. " This should be valid

BiasedMerc commented 3 months ago

Per the contest Readme of Future Integrations " Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold? Yes. " This should be valid

In this specific scenario this does not apply, as the sponsor clearly stated that ERC20 token will not be utilised (as shown in my previous comment showing their direct message in discord). This is like saying that if sponsor says no fee-on-transfer tokens are going to be used, but still all fee-on-transfer issues should still be validated because technically they could still be used in the future.

If sponsor clearly states certain tokens types are not going to be used then they are out of scope and invalid.

sainimukul1911 commented 3 months ago

Per the contest Readme of Future Integrations " Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold? Yes. " This should be valid

In this specific scenario this does not apply, as the sponsor clearly stated that ERC20 token will not be utilised (as shown in my previous comment showing their direct message in discord). This is like saying that if sponsor says no fee-on-transfer tokens are going to be used, but still all fee-on-transfer issues should still be validated because technically they could still be used in the future.

If sponsor clearly states certain tokens types are not going to be used then they are out of scope and invalid.

You may be right but there are clear indications in the protocol for the use of ERC20s. For example all the TransferFrom's and address of tokens. Per the message you quoted the sponsor said only ETH is fully supported but there are mechanisms for handling ERC20s. The example you gave does not apply as there won't be any intended mechanism for handling feeOnTransfer Tokens indicating any future integration. And also per the source of truth hierarchy, contest Readme is the highest source of Truth (hence the future integrations part). Again I might be wrong here with the reasoning, let Hash decide.

joicygiore commented 3 months ago

If the results are valid, the template will be retained for future report submissions

cducrest commented 3 months ago

From "Evert | Sherlock" on discord:

@herethe README has been updated. The exact diff is here https://github.com/sherlock-audit/2024-04-titles/commit/d7f60952df22da00b772db5d3a8272a988546089

Only issues related to native ETH will be accepted. Any issues related to ERC20 tokens will not be accepted.

WangSecurity commented 3 months ago

As mentioned by the Watsons above, the contracts are using only Native ETH, hence, issues about ERC20s should be invalidated. Planning to accept the escalation and invalidate the issue.

0xjuaan commented 3 months ago

@WangSecurity

and leave the issue as it is

you mean invalidate right?

WangSecurity commented 3 months ago

@0xjuaan yes, thank you for notifying

Evert0x commented 3 months ago

Result: Invalid Has Duplicates

sherlock-admin2 commented 3 months ago

Escalations have been resolved successfully!

Escalation status:

sherlock-admin2 commented 3 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/titlesnyc/wallflower-contract-v2/pull/1