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

In exec_create, if unable to transfer value, evm should be returned, not child_evm #70

Open
howlbot-integration bot opened this issue Oct 27, 2024 · 3 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-08 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/instructions/system_operations.cairo#L195-L198

Vulnerability details

Impact

By returning child_evm, Kakarot will continue with the contract cration even though transferring the attached value was unsuccessful

Proof of Concept

Looking at the exec_create function:

    func exec_create{
        ...
    }(evm: model.EVM*) -> model.EVM* {
        ...
        let transfer = model.Transfer(evm.message.address, target_account.address, [value]);
        let success = State.add_transfer(transfer);
        if (success == 0) {
            Stack.push_uint128(0);
            return child_evm;
        }

        return child_evm;
    }

If there is an error in transferring the value, 0 gets pushed to the stack, but the Kakarot VM continues execution in the child_context.
The correct behaviour should be that if there is an error in transferring value to the target address, Kakarot should continue execution in the current call context(not child context).
This way, the contract making the create call will know that there was an error, and handle it appropriately.

Recommended Mitigation Steps

return evm, not child_evm in the highighted code.

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 🤖_primary AI based primary recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Oct 27, 2024
howlbot-integration bot added a commit that referenced this issue Oct 27, 2024
@ClementWalter
Copy link

@ClementWalter ClementWalter added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 4, 2024
@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 Nov 8, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Nov 8, 2024

dmvt changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

c4-judge commented Nov 8, 2024

dmvt marked the issue as grade-b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-08 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants