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.

Sign up for GitHub

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

Jump to bottom

Locked funds within the EntityForging.sol contract due to the forgeWithListed function not handling the potential extra msg.value provided. #501

Closed
howlbot-integration bot opened this issue Aug 9, 2024 · 6 comments
Closed

Locked funds within the EntityForging.sol contract due to the forgeWithListed function not handling the potential extra msg.value provided. #501

howlbot-integration bot opened this issue Aug 9, 2024 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-218 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_54_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityForging/EntityForging.sol#L126

Vulnerability details

Impact

User funds remain locked within the EntityForging.sol contract without the ability to be withdrawn by the user, any other entity, or the owner of the contract. Users will lose their funds without the possibility of a refund. This occurs because the forgeWithListed function does not handle the potential additional msg.value that may be provided during the execution of the forgeWithListed function.

Proof of Concept

To illustrate this issue, consider the following scenario:

  1. A forger is listed with ID: 587 for 0.1 ETH, transaction:
    https://sepolia.etherscan.io/tx/0x2288f0c78c289468be4b2285409b11759622fc6324a76e8164456a4425172ced

  2. A merger is selected to forge with, ID: 592 for 0.15 ETH, transaction:
    https://sepolia.etherscan.io/tx/0x26a20263f7648d393e4a88cc67876317f62fd6f0e0d0e54454e19a4a1100f0cf

  3. 0.05 ETH remains transferred and locked in the EntityForging.sol contract.

Tools Used

Manual review

Recommended Mitigation Steps

Handle the potential extra msg.value by implementing logic within the forgeWithListed function to refund any excess ETH provided during the transaction. This ensures that only the required amount for the forging process is utilized, and any additional funds are returned to the user.

Assessed type

ETH-Transfer
@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_54_group AI based duplicate group recommendation bug Something isn't working duplicate-218 sufficient quality report This report is of sufficient quality labels Aug 9, 2024
howlbot-integration bot added a commit that referenced this issue Aug 9, 2024
@howlbot-integration
CyberscopeInterns issue #501
b315cd2
@c4-judge
Copy link
Contributor

koolexcrypto changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added 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 and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Aug 18, 2024
@c4-judge
Copy link
Contributor

koolexcrypto marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Aug 20, 2024
@c4-judge c4-judge reopened this Aug 31, 2024
@c4-judge c4-judge 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 labels Aug 31, 2024
@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by koolexcrypto

@c4-judge c4-judge added the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Aug 31, 2024
@c4-judge
Copy link
Contributor

koolexcrypto marked the issue as duplicate of #687

@c4-judge
Copy link
Contributor

c4-judge commented Sep 5, 2024

koolexcrypto marked the issue as duplicate of #218

@c4-judge
Copy link
Contributor

c4-judge commented Sep 6, 2024

koolexcrypto changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added 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 and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-218 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_54_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@c4-judge