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

Users don't get refunds when claiming, which allows other users to claim at a discount #112

Closed
howlbot-integration bot opened this issue Sep 6, 2024 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-109 🤖_10_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/main/src/PhiFactory.sol#L283
https://github.com/code-423n4/2024-08-phi/blob/main/src/PhiFactory.sol#L300

Vulnerability details

Impact

Users can claim NFTs through the PhiFactory contract by calling claim(...). If a user sends more ETH with the tx than what is needed for the claiming they are supposed to get a refund of the excess ETH. However, that does not happen and the additional ETH gets stuck in the contract. This allows a cautious user to see that and claim at a discount an NFT due to the ETH left in the contract.

Let's say a user claims an NFT via claim(...) and sends an additional 0.01e18 ETH with their call. This means they won't get their excess ETH back and the ETH will be sitting in the contract. That will give the opportunity to another user to claim an NFT with their ETH at a discount.

Proof of Concept

The following lines of code can be added at the very end of the test_claimMerkle() unit test inside the test/PhiFactory.t.sol file:

phiFactory.claim{ value: totalMintFee }(dataCompressed);
assertEq(address(phiFactory).balance, 0);

phiFactory.claim{ value: totalMintFee + 0.01e18 }(dataCompressed);
assertEq(address(phiFactory).balance, 0.01e18);

phiFactory.claim{ value: totalMintFee - 0.01e18 }(dataCompressed);
assertEq(address(phiFactory).balance, 0);

Tools Used

Manual review

Recommended Mitigation Steps

Make sure to properly refund the user when they call claim(...)

Assessed type

Other

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_10_group AI based duplicate group recommendation bug Something isn't working duplicate-11 sufficient quality report This report is of sufficient quality labels Sep 6, 2024
howlbot-integration bot added a commit that referenced this issue Sep 6, 2024
@c4-judge
Copy link
Contributor

fatherGoose1 marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Sep 12, 2024
@c4-judge
Copy link
Contributor

fatherGoose1 marked the issue as not a duplicate

@c4-judge c4-judge reopened this Sep 23, 2024
@c4-judge
Copy link
Contributor

fatherGoose1 marked the issue as duplicate of #109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-109 🤖_10_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

1 participant