Lines of code

Vulnerability details

Impact

Malicious user can take advantage of the function withdrawFee after the quest end time and successfuly send the quest reward tokens to the protocol fee contract preventing users from claiming their rewards.

Proof of Concept

Every receipt minted should still be able to claim rewards with the function claim even after the end time of the quest.

The below comment is related to the function withdrawRemainingTokens, where the owner withdraws the remaining tokens.
The owner can’t withdraw the tokens for the unclaimed minted receipts, so they can be claimed even after the end time.

contracts/Erc20Quest.sol
       
79: /// @dev Every receipt minted should still be able to claim rewards (and cannot be withdrawn).


contracts/Quest.sol

88:  modifier onlyQuestActive() {
89:        if (!hasStarted) revert NotStarted();
90:        if (block.timestamp < startTime) revert ClaimWindowNotStarted();
91:        _;
92:    }


contracts/Quest.sol

96:  function claim() public virtual onlyQuestActive {
97:        if (isPaused) revert QuestPaused();
98:
99:        uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);
100:
101:       if (tokens.length == 0) revert NoTokensToClaim();
102:
103:       uint256 redeemableTokenCount = 0;
104:       for (uint i = 0; i < tokens.length; i++) {
105:           if (!isClaimed(tokens[i])) {
106:               redeemableTokenCount++;
107:           }
108:       }
109:
110:       if (redeemableTokenCount == 0) revert AlreadyClaimed();
111:
112:       uint256 totalRedeemableRewards = _calculateRewards(redeemableTokenCount);
113:       _setClaimed(tokens);
114:       _transferRewards(totalRedeemableRewards);
115:       redeemedTokens += redeemableTokenCount;
116:
117:       emit Claimed(msg.sender, totalRedeemableRewards);
118:   }

The problem here occurs in the function withdrawFee, this function is made to be called only after the quest end time. The modifier onlyAdminWithdrawAfterEnd is used on it, which reverts if block.timestamp is before the end time. This leads to the main scenario where users who didn’t claim their rewards before the end time of the quest can be exploited by malicious users.

Lets say we have 3 receipt redeemers - Jake, Kiki and Finn:

1. Kiki claimed his rewards before the quest end time, but Jake and Finn didn’t.
2. Malicious user can call the function withdrawFee numerous number of times as there is no check to see if the protocol fee was already sent or if the msg.sender calling the function is the owner.
3. As a result the malicious user can send the contract rewards allocated for Jake and Finn to the protocol fee recipient.

This scenario leaves receipt redeemers like Finn and Jake unable to claim their rewards.

Duo to this problem, malicious users can sabotage the people who didn’t claim their receipts before the quest end time,
by calling the function withdrawFee numerous number of times and successfuly sending the quest contract balance to the protocol fee recipient. As a receipt can be claimed only on its allocated quest contract, this scenario leaves users unable to claim their rewards.

contracts/Erc20Quest.sol

102:  function withdrawFee() public onlyAdminWithdrawAfterEnd {
103:      IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
104:  }


contracts/Quest.sol

76:  modifier onlyAdminWithdrawAfterEnd() {
77:        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
78:        _;
79:    }

Tools Used

Manual review

Recommended Mitigation Steps

Consider applying the onlyOwner modifier on the function withdrawFee, so the function can be called only by the owner.

102:  function withdrawFee() public onlyOwner onlyAdminWithdrawAfterEnd {
103:      IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
104:  }  

---

The text was updated successfully, but these errors were encountered:

All reactions