Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgraded Q -> 3 from #621 [1675724753994] #696

Closed
c4-judge opened this issue Feb 6, 2023 · 5 comments
Closed

Upgraded Q -> 3 from #621 [1675724753994] #696

c4-judge opened this issue Feb 6, 2023 · 5 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly unsatisfactory does not satisfy C4 submission criteria; not eligible for awards withdrawn by judge Special case: this finding was auto-generated by a judge and is now withdrawn; it can be ignored

Comments

@c4-judge
Copy link
Contributor

c4-judge commented Feb 6, 2023

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

L1 - Owner could withdraw all unclaimed tokens while some still should be claimable
withdrawRemainingTokens() function in the Erc1155Quest contract allows the owner to withdraw all remaining tokens, including unclaimed ones that may still be claimable in the future. This could result in the accidental withdrawal of tokens that are meant to remain on the contract balance until claimed by users.

File: Erc1155Quest.sol
52: /// @dev Withdraws the remaining tokens from the contract. Only able to be called by owner
53: /// @param to_ The address to send the remaining tokens to
54: function withdrawRemainingTokens(address to_) public override onlyOwner {
55: super.withdrawRemainingTokens(to_);
56: IERC1155(rewardToken).safeTransferFrom(
57: address(this),
58: to_,
59: rewardAmountInWeiOrTokenId,
60: IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
61: '0x00'
62: );
63: }
Recommended Mitigation Steps

Consider adding tracking flow in Erc1155Quest contract withdrawRemainingTokens function similar to Erc20Quest withdrawing function:

function withdrawRemainingTokens(address to_) public override onlyOwner { 
    super.withdrawRemainingTokens(to_);
    uint256 remainingTokens = totalParticipants - questFactoryContract.getNumberMinted(questId);
    IERC1155(rewardToken).safeTransferFrom(
        address(this),
        to_,
        rewardAmountInWeiOrTokenId,
        remainingTokens,
        '0x00'
    );
}
@c4-judge c4-judge added the 3 (High Risk) Assets can be stolen/lost/compromised directly label Feb 6, 2023
@c4-judge
Copy link
Contributor Author

c4-judge commented Feb 6, 2023

kirk-baird marked the issue as duplicate of #42

@c4-judge c4-judge closed this as completed Feb 6, 2023
@c4-judge c4-judge added duplicate-42 downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax withdrawn by judge Special case: this finding was auto-generated by a judge and is now withdrawn; it can be ignored and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Feb 6, 2023
@c4-judge
Copy link
Contributor Author

This auto-generated issue was withdrawn by kirk-baird

@c4-judge c4-judge reopened this Feb 10, 2023
@c4-judge
Copy link
Contributor Author

This previously downgraded issue has been upgraded by kirk-baird

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly duplicate-528 and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax duplicate-42 labels Feb 10, 2023
@c4-judge
Copy link
Contributor Author

kirk-baird marked the issue as not a duplicate

@c4-judge
Copy link
Contributor Author

kirk-baird marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly unsatisfactory does not satisfy C4 submission criteria; not eligible for awards withdrawn by judge Special case: this finding was auto-generated by a judge and is now withdrawn; it can be ignored
Projects
None yet
Development

No branches or pull requests

1 participant