Platform: Code4rena
Start Date: 25/01/2023
Pot Size: $36,500 USDC
Total HM: 11
Participants: 173
Period: 5 days
Judge: kirk-baird
Total Solo HM: 1
Id: 208
League: ETH
Rank: 41/173
Findings: 3
Award: $119.15
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: adriro
Also found by: 0xRobocop, 0xmrhoodie, 0xngndev, AkshaySrivastav, ArmedGoose, Atarpara, Bauer, CodingNameKiki, ElKu, Garrett, HollaDieWaldfee, IllIllI, Iurii3, KIntern_NA, KmanOfficial, Lotus, M4TZ1P, MiniGlome, Ruhum, SovaSlava, bin2chen, bytes032, carrotsmuggler, cccz, chaduke, codeislight, cryptonue, doublesharp, evan, fs0c, glcanvas, gzeon, hansfriese, hihen, hl_, holme, horsefacts, ladboy233, lukris02, mahdikarimi, manikantanynala97, martin, mert_eren, mrpathfindr, omis, peakbolt, peanuts, prestoncodes, rbserver, rvierdiiev, sashik_eth, timongty, tnevler, trustindistrust, usmannk, wait, yixxas, zadaru13, zaskoh
0.7512 USDC - $0.75
Once the Quest endTime is passed, anyone can call the withdrawFee() function multiple times. This means that the protocol fee can be sent to the protocolFeeRecipient several times, thus draining the Quest rewardToken. So the protocolFeeRecipient can collect all the remaining funds and tokens that have not already be claimed. Therfore, a malicious actor can prevent any player to claim their rewardTokens after the end of the Quest.
Erc20Quest.sol:
L102: function withdrawFee() public onlyAdminWithdrawAfterEnd { IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee()); }
// Withdraw nearly all the remaining tokens while(IERC20(rewardToken).balanceOf(address(Erc20Quest)) > Erc20Quest.protocolFee()) { Erc20Quest.withdrawFee(); }
withdrawFee() should be callable only once.
bool feeWithdrawn; // [...] function withdrawFee() public onlyAdminWithdrawAfterEnd { require(!feeWithdrawn, "Fees already withdrawn"); IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee()); feeWithdrawn = true; }
#0 - c4-judge
2023-02-05T12:05:35Z
kirk-baird marked the issue as duplicate of #23
#1 - c4-judge
2023-02-14T08:54:38Z
kirk-baird changed the severity to 3 (High Risk)
#2 - c4-judge
2023-02-14T08:56:42Z
kirk-baird marked the issue as satisfactory
🌟 Selected for report: RaymondFam
Also found by: 0xMirce, AkshaySrivastav, AlexCzm, Aymen0909, BClabs, CodingNameKiki, ElKu, HollaDieWaldfee, Josiah, KIntern_NA, MiniGlome, StErMi, adriro, bin2chen, cccz, chaduke, csanuragjain, gzeon, hihen, holme, libratus, minhquanym, omis, peakbolt, peanuts, rbserver, rvierdiiev, timongty, ubermensch, usmannk, wait, zaskoh
7.046 USDC - $7.05
Judge has assessed an item in Issue #599 as 3 risk. The relevant finding follows:
[L-01] Erc1155Quest's tokens can be withdrawn before every reward has been claimed Impact The owner can withdraw all the remaining tokens after the Quest endTime. Thus, users who have not claimed their reward at the end of the quest may not be able to do so because the tokens can be withdrawn by the owner beforehand.
Proof Of Concept The withdrawRemainingTokens() function withdraws all token balance whithout checking unclaimed tokens.
File: Erc1155Quest.sol
L56: IERC1155(rewardToken).safeTransferFrom( address(this), to_, rewardAmountInWeiOrTokenId, IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId), '0x00' );
#0 - c4-judge
2023-02-06T22:46:00Z
kirk-baird marked the issue as duplicate of #42
#1 - c4-judge
2023-02-10T05:03:11Z
This auto-generated issue was withdrawn by kirk-baird
#2 - c4-judge
2023-02-10T05:12:14Z
This previously downgraded issue has been upgraded by kirk-baird
#3 - c4-judge
2023-02-14T09:25:35Z
kirk-baird marked the issue as satisfactory
#4 - c4-judge
2023-02-23T23:49:21Z
kirk-baird changed the severity to 2 (Med Risk)
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0xAgro, 0xSmartContract, 0xhacksmithh, 0xngndev, Aymen0909, Bnke0x0, Breeje, Deivitto, Diana, Dug, Iurii3, LethL, MiniGlome, NoamYakov, RaymondFam, ReyAdmirado, Rolezn, SAAJ, adriro, ali, arialblack14, atharvasama, c3phas, carlitox477, catellatech, chaduke, cryptonue, cryptostellar5, ddimitrov22, dharma09, doublesharp, favelanky, georgits, glcanvas, gzeon, halden, horsefacts, jasonxiale, joestakey, karanctf, lukris02, matrix_0wl, nadin, navinavu, saneryee, shark, thekmj
111.3544 USDC - $111.35
This report does not include publicly known issues
| Issue | Instances | |
|---|---|---|
| [GAS-01] | Comparing strings is not gas efficient | 2 |
| [GAS-02] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 1 |
| [GAS-03] | ++<x>/<x>++ should be unchecked when it is no possible for them to overflow | 10 |
| [GAS-04] | Optimize names to save gas | 4 |
| [GAS-05] | internal functions only called once can be inlined to save gas | 1 |
| [GAS-06] | Setting the constructor to payable | 6 |
| [GAS-07] | Use </> instead of >=/>= | 3 |
| [GAS-08] | Don't compare boolean expressions to boolean literals | 3 |
Total: 30 instances over 8 issues
It is not gas efficient to compare contractType_ as a string, as the keccak256 function is used twice to convert the string to a bytes32 value, and then to compare the two bytes32 values. It would be more efficient to compare contractType_ as an integer, which requires less computation and thus less gas.
File: QuestFactory.sol L72: if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20'))) L105: if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155')))
Recommended Mitigation Steps
// Costs around 2000 less gas if (contractType_ == 1)
Alternatively keccak256(abi.encodePacked('erc20'))) and keccak256(abi.encodePacked('erc1155'))) could be pre-computed.
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
File: Quest.sol 115: redeemedTokens += redeemableTokenCount;
++<x>/<x>++ should be unchecked when it is no possible for them to overflowThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP
File: QuestFactory.sol L101: ++questIdCount; L132: ++questIdCount; L226: quests[questId_].numberMinted++;
File: Quest.sol L70: for (uint i = 0; i < tokenIds_.length; i++) { L104: for (uint i = 0; i < tokens.length; i++) { L106: redeemableTokenCount++;
File: RabbitHoleReceipt.sol L117: for (uint i = 0; i < msgSenderBalance; i++) { L121: foundTokens++; L128: for (uint i = 0; i < msgSenderBalance; i++) { L131: filterTokensIndexTracker++;
Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.
See more here
File: Quest.sol File: Erc20Quest.sol File: Erc1155Quest.sol File: RabbitHoleReceipt.sol
Recommended Mitigation Steps Find a lower method ID name for the most called functions for example b_A6Q() is cheaper than b().
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
File: QuestFactory.sol L152: function grantDefaultAdminAndCreateQuestRole(address account_) internal {
payableSaves ~13 gas per instance
File: QuestFactory.sol L35: constructor() initializer {}
File: Quest.sol L26: constructor([...]) {
File: Erc20Quest.sol L17: constructor([...]) Quest([...]){
File: Erc1155Quest.sol L13: constructor([...]) Quest([...]){
File: RabbitHoleReceipt.sol L39: constructor(){
File: RabbitHoleTickets.sol L28: constructor(){
</> instead of >=/>=In Solidity, there is no single op-code for ≤ or ≥ expressions. What happens under the hood is that the Solidity compiler executes the LT/GT (less than/greater than) op-code and afterwards it executes an ISZERO op-code to check if the result of the previous comparison (LT/ GT) is zero and validate it or not. Example:
// Gas cost: 21391 function check() exernal pure returns (bool) { return 3 > 2; }
// Gas cost: 21394 function check() exernal pure returns (bool) { return 3 >= 3; }
The gas cost between these contract differs by 3 which is the cost executing the ISZERO op-code, making the use of < and > cheaper than ≤ and ≥.
File: Quest.sol L35: if (endTime_ <= block.timestamp) revert EndTimeInPast(); L36: if (startTime_ <= block.timestamp) revert StartTimeInPast(); L37: if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime();
if (<x> == true) => if (<x>), if (<x> == false) => if (!x>)
File: QuestFactory.sol L73: if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed(); L221: if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
File: Quest.sol L136: return claimedList[tokenId_] == true;
#0 - c4-judge
2023-02-15T21:51:34Z
kirk-baird marked the issue as grade-a