RabbitHole Quest Protocol contest - fs0c's results

Lines of code

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

Vulnerability details

Impact

The withdrawFee function in Erc20Quest is used to transfer the protocolFee to protocolFeeRecipient . This function has access control issues. The modifier onlyAdminWithdrawAfterEnd only checks if the function is called before endTime or not and reverts if it is.

modifier onlyAdminWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    } 

This function is missing the onlyOwner modifier.

Due to this issue, it can be called multiple times by anyone to transfer funds to the protocolFeeRecipient, and the legitimate users would not be able to claim their tokens as there won’t be sufficient funds in the contract anymore.

Even if the protocolFeeRecipient transfer funds back to this contract, a malicious user with an objective to not let users claim their tokens, can front-run their claim call to transfer funds back to the protocolFeeRecipient and get their transactions reverted.

POC

function withdrawFee() public onlyAdminWithdrawAfterEnd {
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee()); //@audit anyone can call multiple times
    }

Recommendation Mitigation

Just add onlyOwner modifier.

Lines of code

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

Vulnerability details

Impact

The withdrawFee function in Erc20Quest is used to transfer the protocolFee to protocolFeeRecipient . This function has access control issues. The modifier onlyAdminWithdrawAfterEnd only checks if the function is called before endTime or not and reverts if it is.

modifier onlyAdminWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    } 

This function is missing the onlyOwner modifier.

Due to this issue, it can be called multiple times by anyone to transfer funds to the protocolFeeRecipient, and the legitimate users would not be able to claim their tokens as there won’t be sufficient funds in the contract anymore.

Even if the protocolFeeRecipient transfer funds back to this contract, a malicious user with an objective to not let users claim their tokens, can front-run their claim call to transfer funds back to the protocolFeeRecipient and get their transactions reverted.

POC

function withdrawFee() public onlyAdminWithdrawAfterEnd {
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee()); //@audit anyone can call multiple times
    }

Recommendation Mitigation

Just add onlyOwner modifier.

Lines of code

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

Vulnerability details

Impact

In the current implementation, the function withdrawFee can be called multiple times. It should only be allowed to called once. Calling more than once would let owner steal from legit users as there won’t be enough funds left for legit users to claim tokens if owner calls this multiple times.

Note: This is different from the high severity issue where it can be called by anyone.

POC

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

Recommendation

Let the function only be called once.