RabbitHole Quest Protocol contest - wait's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L100-L104

Vulnerability details

Impact

Any malicious user can transfer the remaining funds from any Erc20Quest contract by repeatedly calling the Erc20Quest.sol#withdrawFee function

Proof of Concept

Function Erc20Quest.sol#withdrawFee can only be called after the endTime, but does not limit the number of times it can be called or who can call it.

Here is the function code:

/// @notice Sends the protocol fee to the protocolFeeRecipient
/// @dev Only callable when the quest is ended
function withdrawFee() public onlyAdminWithdrawAfterEnd {
    IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
}

Tools Used

Manual

Recommended Mitigation Steps

We can Erc20Quest.sol#withdrawFee to be called only once by adding a flag in the storage, and change the function like this:

function withdrawFee() public onlyAdminWithdrawAfterEnd {
    // begin add
    require(!withdrawFlag);
    withdrawFlag = true;
    // end add
    IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
}

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81

Vulnerability details

Impact

If Erc1155Quest.sol#withdrawRemainingTokens is correct, then Erc20Quest.sol#withdrawRemainingTokens y will cause the owner to withdraw less tokens. If Erc20Quest.sol#withdrawRemainingTokens is correct, then Erc1155Quest.sol#withdrawRemainingTokens will cause the unclaimed users lose the unclaimed tokens.

Proof of Concept

Function Erc1155Quest.sol#withdrawRemainingTokens will withdraw all remaining tokens to the owner:

IERC1155(rewardToken).safeTransferFrom(
    address(this),
    to_,
    rewardAmountInWeiOrTokenId,
    IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
    '0x00'
);

While function Erc20Quest.sol#withdrawRemainingTokens will not withdraw the unclaimed tokens:

uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);

Tools Used

Manual

Recommended Mitigation Steps

We only need to change one of the two functions to make them logically consistent.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219

Vulnerability details

Impact

Malicious users can call QuestFactory.sol#mintReceipt on different chains use a same signature.

Proof of Concept

The signature validated in function QuestFactory.sol#mintReceipt is obtained by signing the message hash: keccak256(abi.encodePacked(msg.sender, questId_)).

The signing data is too simple. If a malicious user obtains a signature, it can be reused in any other chain or other contract with the same functionality, as long as the claimSignerAddress and questId are the same.

Tools Used

Manual

Recommended Mitigation Steps

I recommend the eip-712 to fix this issue.