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

1 stars 2 forks source link

ERC20 quest does not support rebasing tokens #454

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L66-L68 https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L81-L87 https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L102-L104

Vulnerability details

Impact

When a rebasing token is used as the reward token for an ERC20 quest, it is possible that this token's rebasing event, which reduces its balance owned by the Erc20Quest contract, occurs before the following Erc20Quest._transferRewards, Erc20Quest.withdrawRemainingTokens, and Erc20Quest.withdrawFee functions are called. When this occurs, calling these transfer functions can revert due to insufficient balance of this rebasing reward token owned by the Erc20Quest contract. As a result, the reward token amounts that suppose to be received by the RabbitHole receipt holders, quest owner, and/or protocol fee recipient cannot be transferred.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L66-L68

    function _transferRewards(uint256 amount_) internal override {
        IERC20(rewardToken).safeTransfer(msg.sender, amount_);
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L81-L87

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

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

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L102-L104

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

Proof of Concept

The following steps can occur for one of the described scenarios.

  1. An ERC20 quest is created for one total participant and zero protocol fee with a rebasing token as its reward token.
  2. Alice is provided with the signature for minting one RabbitHole receipt for this quest. Then, she mints one receipt accordingly.
  3. Before Alice claims the rewards associated with the minted receipt, the reward token's rebasing event occurs that reduces its balance owned by the Erc20Quest contract.
  4. Alice calls the claim function for this quest but this function call reverts because the rebasing reward token's balance owned by the Erc20Quest contract is now insufficient to cover her reward token amount. Hence, she is unable to receive the reward token amount associated with her RabbitHole receipt.

Tools Used

VSCode

Recommended Mitigation Steps

Like some other protocols, this protocol does not have to support rebasing tokens. A blocklist can be used to block the usage of these tokens. If blocking these tokens, please explicitly document it so users know about this.

c4-judge commented 1 year ago

kirk-baird changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded 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 duplicate of #630

c4-judge commented 1 year ago

kirk-baird marked the issue as satisfactory

c4-judge commented 1 year ago

kirk-baird marked the issue as selected for report

c4-sponsor commented 1 year ago

waynehoover marked the issue as sponsor disputed

waynehoover commented 1 year ago

We have a rewardAllowlist in which we will not include rebasing tokens, our protocol does not support rebasing tokens.

kirk-baird commented 1 year ago

Reward allow list is a genuine solution to prevent rebasing and FoT tokens and so I'm going to downgrade this to a QA.

I'd also recommend clearly documenting the rebasing and FoT tokens will not be allowed.

c4-judge commented 1 year ago

kirk-baird changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

kirk-baird marked the issue as grade-a

c4-judge commented 1 year ago

kirk-baird marked the issue as not selected for report