User receives proportional reward for burning contrary to docs

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L230-L233

Vulnerability details

Impact

Comment in code states that user doesn't receive reward for his own burn, similar to buying.
However actually he does receive claimable fees for this burn. As a result, actual fee paid for burn is less, because user receives part of it in return.

Proof of Concept

Here you can see incorrect order of operations: it firstly increases shareData[_id].shareHolderRewardsPerTokenScaled by fees, then calculates user's rewards

    function burnNFT(uint256 _id, uint256 _amount) external {
        uint256 fee = getNFTMintingPrice(_id, _amount);

        SafeERC20.safeTransferFrom(token, msg.sender, address(this), fee);
@>      _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        // The user does not get the proportional rewards for the burning (unless they have additional tokens that are not in the NFT)
@>      uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
        tokensByAddress[_id][msg.sender] += _amount;
        shareData[_id].tokensInCirculation += _amount;
        _burn(msg.sender, _id, _amount);

        SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim);
        // ERC1155 already logs, but we add this to have the price information
        emit NFTsBurned(_id, msg.sender, _amount, fee);
    }

But must firstly calculate user's rewards, and only after split fees

Tools Used

Manual Review

Recommended Mitigation Steps

    function burnNFT(uint256 _id, uint256 _amount) external {
        uint256 fee = getNFTMintingPrice(_id, _amount);

        SafeERC20.safeTransferFrom(token, msg.sender, address(this), fee);
+       // The user does not get the proportional rewards for the burning (unless they have additional tokens that are not in the NFT)
+       uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
-       // The user does not get the proportional rewards for the burning (unless they have additional tokens that are not in the NFT)
-       uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
        ...
    }

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as duplicate of #241

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[c4-judge]

MarioPoneder marked the issue as duplicate of #9

[c4-judge]

MarioPoneder marked the issue as satisfactory

[d3e4]

This is not a duplicate of #9 and it is invalid. When fees are accrued the burner has not yet received his token on which to accrue rewards, so it all goes to previous holders.

[T1MOH593]

Agree with above statement

[MarioPoneder]

Thank you for your comments!

This was indeed duplicated too quickly and I admit to have overlooked it since the Recommended Mitigation Steps seem to have identified the same core issue as #9, which is in fact not that case.

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[c4-judge]

MarioPoneder marked the issue as unsatisfactory:
Invalid