code-423n4 / 2023-01-rabbithole-findings

1 stars 2 forks source link

Upgraded Q -> 3 from #664 [1675726122175] #701

Closed c4-judge closed 1 year ago

c4-judge commented 1 year ago

Judge has assessed an item in Issue #664 as 3 risk. The relevant finding follows:

[L-2] ERC1155 Quest: withdrawRemainingTokens should factor in total number of receipts minted before withdrawal

Issue: There may be users with unredeemed receipts who will not be able to claim if all the remaining tokens are withdrawn to the owner after a quest has ended. Ideally, the process should be the same as ERC20 where this is taken into account.

Suggested Fix: Include the Quest Factory contract in the ERC1155 contract as well (e.g. through the constructor similar to receiptContractAddress_)

In withdrawRemainingTokens() function:

uint unclaimedTokens       = (receiptRedeemers() - redeemedTokens);
uint256 nonClaimableTokens = IERC1155(rewardToken).balanceOf(address(this)) - unclaimedTokens;
IERC1155(rewardToken).safeTransferFrom(address(this), to_, nonClaimableTokens, nonClaimableTokens, '0x00');

/// @notice Call the QuestFactory contract to get the amount of receipts that have been minted
/// @return The amount of receipts that have been minted for the given quest
    function receiptRedeemers() public view returns (uint256) {
        return questFactoryContract.getNumberMinted(questId);
 }

Link to Github Reference: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L52-L63

c4-judge commented 1 year ago

kirk-baird marked the issue as duplicate of #42

c4-judge commented 1 year ago

This auto-generated issue was withdrawn by kirk-baird

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by kirk-baird

c4-judge commented 1 year ago

kirk-baird marked the issue as satisfactory

c4-judge commented 1 year ago

kirk-baird changed the severity to 2 (Med Risk)