RabbitHole Quest Protocol contest - ArmedGoose's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L76

Vulnerability details

Impact

function withdrawFee() in Erc20Quest.sol allows to withdraw the protocol fee to the recipient set in contract. Generally it should be an administrative operations (the owners decide when to perform fee withdrawal), which also can be confirmed by looking at modifier onlyAdminWithdrawAfterEnd Also, in case of a compromise of that address (fee recipient) potential attacker has easy way to just obtain protocol fees there.

The issue is function is public due to lack of owner check in modifier named onlyAdminWithdrawAfterEnd.

However

Proof of Concept

The modifier is here: https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L76 and it only checks for quest end, and not for being an admin. Function withdrawFee https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L102 can be invoked by anyone, as long as quest ended. Note, that if that modifier in current shape is used in the future in other operations, this may lead to serious security flaws.

Tools Used

Manual approach

Recommended Mitigation Steps

Add ownership check to that modifier.

Judge has assessed an item in Issue #206 as 2 risk. The relevant finding follows:

Issue 2: receipts are not burned upon claiming reward.

In my opinion, current system of just claiming some tokens to be "used" has some downsides:

in any claim check, users spend gas to iterate over these tokens too etc. there are two loops in only claim function that checks every token of user

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L114

Vulnerability details

Impact

State change is performed after call to _transferRewards, which under the hood executes _safeTransferFrom to the sender, which may also be a contract. Function claim() also does not have nonReentrant modifier, which might allow malicious users to reenter the function for example if transferred token contains a Callback function (e.g. onTokenReceive etc.).

Proof of Concept

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L114

Tools Used

Manual approach

Recommended Mitigation Steps

Change order, put current line 115 before current line 114 so no state updates are performed after call to external contract.

SVG is rendering with unsanitized special charaters. The rendering function is using user input (string) (string memory questId_) to output an SVG. This might be lead to issues in the future, since SVG format allows for embedding javascript scripts inside, so potentially, if that function has any other use in the future, it might be abused to generate malicious images. However, at the moment probability is low, since it uses existing questId is created by administrative role, and anyone in posession of administrative role have plenty of other attack vectors than xssing users.

Occurence: https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/ReceiptRenderer.sol#L100

Remediation: Filter special characters, at least <, >, ", ' ideally allow only alphanumeric ones in the function.

References: https://github.com/code-423n4/2022-01-timeswap-findings/issues/131 https://securiumsolutions.com/blog/xss-through-svg-file-upload/

Issue 2: receipts are not burned upon claiming reward.

In my opinion, current system of just claiming some tokens to be "used" has some downsides:

• in any claim check, users spend gas to iterate over these tokens too etc. there are two loops in only claim function that checks every token of user
• depending on how the front end will be organized, how users on secondary markets will be warned no to buy already claimed receipts? This might be primary vector for scammers

Recommendation: the receipts should be burned upon reward is claimed for them, removing them from the circulating supply. This decreases gas consumption on users side and prevents potential scam attack vectors in the future.