xiaoming90 - Assets can be pulled and stolen from user's wallet

[sherlock-admin3]

xiaoming90

high

Assets can be pulled and stolen from user's wallet

Summary

Attackers can exploit the vulnerable FeeManager contract to steal assets from the user's wallet.

Vulnerability Detail

The issue with the FeeManager.collectCreationFee and FeeManager.collectMintFee functions is that these functions are permissionless and can be executed by anyone. They also allow the caller to define an arbitrary feePayer_, which will lead to the issue described in the later section.

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L166

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:     }

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L183

File: FeeManager.sol
183:     function collectMintFee(
184:         IEdition edition_,
185:         uint256 tokenId_,
186:         uint256 amount_,
187:         address payer_,
188:         address referrer_
189:     ) external payable {
190:         _collectMintFee(
191:             edition_, tokenId_, amount_, payer_, referrer_, getMintFee(edition_, tokenId_, amount_)
192:         );
193:     }

Assume Bob grants the maximum allowance to the FeeManger contract or there is a residual allowance due to a previous transaction.

Note that Bob has to grant the allowance to FeeManager because when he creates a new collection/work or mints a token, the FeeManager.collectCreationFee or FeeManager.collectMintFee function will attempt to pull the fee from the payer, which is pointed to Bob's wallet. If Bob does not grant the allowance to FeeManager, the pulling of tokens from Bob's wallet will fail and the transaction will revert.

Assume a malicious user called Alice is one of the fee recipients. She can call the FeeManager.collectCreationFee or FeeManager.collectMintFee function directly with the payer set to Bob's wallet address. The FeeManager will pull the assets from Bob's wallet and route the fee to Alice's wallet. She repeats this action multiple times until Bob's wallet is completely drained.

Impact

Loss of assets for the victim as their wallets can be drained by attackers.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L166

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L183

Tool used

Manual Review

Recommendation

To remediate the issue, the following must be enforced:

1. The FeeManager.collectCreationFee and FeeManager.collectMintFee functions must not be permissionless. They should only be restricted to the TitlesCore and Edition contract, respectively.
2. The users/public should not be allowed to define the payer of the FeeManager.collectCreationFee and FeeManager.collectMintFee functions, and should not be given an option to choose the payer. The payer must be set to the caller (msg.sender) themselves within the creation and minting function.

Consider the following changes:

function collectCreationFee(IEdition edition_, uint256 tokenId_, address feePayer_)
    external
    payable
+       onlyTitlesCore    
{
    Fee memory fee = getCreationFee();
    if (fee.amount == 0) return;

    _route(fee, Target({target: protocolFeeReceiver, chainId: block.chainid}), feePayer_);
    emit FeeCollected(address(edition_), tokenId_, ETH_ADDRESS, fee.amount, 0);
}

function collectMintFee(
    IEdition edition_,
    uint256 tokenId_,
    uint256 amount_,
    address payer_,
    address referrer_
- ) external payable {
+ ) external payable onlyEditionContract {
    _collectMintFee(
        edition_, tokenId_, amount_, payer_, referrer_, getMintFee(edition_, tokenId_, amount_)
    );
}

function collectMintFee(
    IEdition edition,
    uint256 tokenId_,
    uint256 amount_,
    address payer_,
    address referrer_,
    Strategy calldata strategy_
- ) external payable {
+ ) external payable onlyEditionContract {
    _collectMintFee(
        edition, tokenId_, amount_, payer_, referrer_, getMintFee(strategy_, amount_)
    );
}

In addition, ensure that all contracts that execute the FeeManager.collectCreationFee and FeeManager.collectMintFee functions set the payer to the caller (msg.sender) under all scenarios.

Duplicate of #251

[pqseags]

46 has more clear details, but this one is somewhat unclear. For ERC20 tokens with approval this could happen, but ERC20 is out of scope. For ETH, it could only drain residual money that is in the contract, which is still an issue, but not quite what this ticket is describing.

[WangSecurity]

I believe this issue is not a duplicate of #251 because they have differen root causes, vulnerability paths, impacts and fixes. Hence, planning to create a new issue family of high severity where the users can freely call permissionless functions and steal the fees from FeeManager with this report as the main.

Duplicates are:

139, #383, #425.

[ZdravkoHr]

@WangSecurity, what's the difference between this and #251? Why is this valid, but #251 and its dups were invalidated?

[WangSecurity]

@ZdravkoHr thank you for asking and indeed you're correct and I've overlooked it. This report is indeed also invalid under the same reason: the report is about ERC-20 tokens which are not used in the current version of the protocol. Hence, the correct family will be #425 as the main report and #139 + #383 as duplicates.

[ZdravkoHr]

@WangSecurity, so the only difference between the valid and invalid ones is that the invalid ones don't mention native token, but ERC20 or I am still missing something?

[0rpse]

@WangSecurity i think #357 too should be duped with #425

[WangSecurity]

@ZdravkoHr correct, the ERC20 tokens are out of scope for this contest, hence, issues regarding ERC20 tokens are default. If you think I'm missing some point or duplicates, please share it.

[ZdravkoHr]

@WangSecurity, as far as I understand both type of reports find the core vulnerability which is unauthorized transfer of funds. Is the fact that the first group uses ETH as example and the second one - ERC20 - really such a big factor to result in invalidating the second group?

[WangSecurity]

That's a very good question @ZdravkoHr and I understand where it comes from. But, I believe it wouldn't be fair to reward the reports about ERC20 tokens, even though they were excluded mid-contest and similar in the attack path. Similar to situations when the README says "any" token will be used and we exclude the ones about weird behaviour of USDT and USDC, cause any means any without weird traits. And here the vulnerability exists for both ETH and ERC20 tokens, but ERC20 tokens are excluded. Hope you understand my reasoning.

[ZdravkoHr]

@WangSecurity, I understand, but I want to add something before you make your final decision. In the example you provided if I report a vulnerability about a fee-on-transfer token (when the readme listed any), this vulnerability will technically not exist because the standard tokens don't have this fee mechanism.

While in this situation the vulnerability still exists for native tokens with the same attack path. Following this logic, if a protocol states they will use WETH and USDC only and there are reports for invalid decimals handling that use USDT as example, these reports should be invalided because USDT will not be used, even though both it and USDC have 6 decimals.

[WangSecurity]

For the first question, yes, if the weird behaviour wasn't explicitly mentioned in the README, then reports about fee-on-transfer would be invalid.

For the second, it's actually a good one, but I would say yes, reports about USDT would be invalid, if the README says only ETH and USDC are used. I understand that it sounds quite unfair, but from my side it would look like the watson didn't read the README or just copy-pasted another issue. Hence, they didn't put enough effort to be rewarded. I understand it's some kind of a double-edge sword, but rn I would judge it as insufficient duplicate, but depends on the situation.

Still it doesn't change the fact that in this contest only ETH is used, hence issues about ERC-20 are not valid.