RabbitHole Quest Protocol contest - simon135's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/068d628f019e9469aecbf676370075c1f6c980fd/contracts/Quest.sol#L99

Vulnerability details

Impact

It is possible that an attacker can cause users who are trying to claim tokens, to be dosed (out of gas)

Proof of Concept

When a user calls claim function, it figures out how many tokens the msg.sender owns. https://github.com/rabbitholegg/quest-protocol/blob/bd213e8629bb8587dd4bb35f3e9e8f8e42b40336/contracts/RabbitHoleReceipt.sol#L109-L118

        uint msgSenderBalance = balanceOf(claimingAddress_);
        uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
        uint foundTokens = 0;

        for (uint i = 0; i < msgSenderBalance; i++) {

https://github.com/rabbitholegg/quest-protocol/blob/068d628f019e9469aecbf676370075c1f6c980fd/contracts/Quest.sol#L96-L104

      uint256[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);

        if (tokens.length == 0) revert NoTokensToClaim();

        uint256 redeemableTokenCount = 0;
        for (uint256 i = 0; i < tokens.length; i++) {
            if (!isClaimed(tokens[i])) {
                redeemableTokenCount++;
            }

this goes through the tokens and assigns them claimed https://github.com/rabbitholegg/quest-protocol/blob/068d628f019e9469aecbf676370075c1f6c980fd/contracts/Quest.sol#L113

      for (uint256 i = 0; i < tokenIds_.length; i++) {

these 3 code snippets loop over msg.sendertokens. since it loops over tokens that a user owns an attacker can transfer claimed tokens to a user and cause them high gas costs and possibly revert (out of gas) ex: Alice(attacker) bob(victim) 5000 recipient tokens Alice owns 1000

1. Alice claims all of them and gets the reward
2. Alice transfers them to Bob (right before bob calls the claim function
3. Bob runs the tx and it reverts ( out of gas)

another scenario would be where token owners want to gang up on Bob so they cause bob to revert
Bob can only claim rewards if he sends the tokens to another address or burns them But doing so would still cost a lot of gas.

Tools Used

Vim

Recommended Mitigation Steps

Make the claim function based on user input tokenId which then the function can do the checking with out the high gas cost

///pseudocode
function claim(uint tokenId)
if (isclaimed(tokenId){
revert
}

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/068d628f019e9469aecbf676370075c1f6c980fd/contracts/Quest.sol#L113-L116

Vulnerability details

Impact

If the reward token is erc1155/erc777 an attacker can reenter and then buy/transfer another unclaimed token to the attacker address and then the var redeemTokens wont be equal to how many tokens were actually redeemed.

Proof of Concept

ex: reward token is an erc1155 that has _afterTokenTransfer Alice(attacker) has 2 receipt tokens, the first one is on a smart contract that will do the reentrancy, and the second one is on Alice's address but is approved to transfer to the smart contract(the own that holds the first receipt)

1. Alice calls the sc to claim rewards

 IERC1155(rewardToken).safeTransferFrom(address(this), msg.sender, rewardAmountInWeiOrTokenId, amount_, '0x00');

2. _afterTokenTransfer which causes the sc to call a function in its fallback function that transfers the approved token to the sc

   try IERC1155Receiver(to).onERC1155Received(operator, from, id, amount, data) returns (bytes4 response) {

3. We then reenter with recipient,not yet claimed token and we claim it result: the invariant that redeemedTokens = tokens that are redeemed is false because it doesn't account for the first token that we reentered. The issue is worse with erc777 tokens because of the fact that accounting will be in the withdrawRemainingTokens function

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

after the reentrancy ex: redeemedTokens=9 but should be 10 receiptRedeemers()=12 rewardAmountInWeiOrTokenId=1e5 unclaimedTokens=300000 assuming they are some tokens left balance(address(this)=201000 and protocolFee=500 nonClaimableTokens=201000 - 500 - 300000 it would revert ( negative numbers with uint) and funds would be stuck in the contract forever The real estimate for nonClaimableTokens=201000-500-200000=500 and the owner can get funds out but 500 wei will be lost in the contract and it can get worse with large amounts of quests and the attacker reentering multiple times to cause a bigger gap between the real redeemedTokens

Tools Used

Recommended Mitigation Steps

add nonReentrancy modifier