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 edge cases, user might lose funds when sending l1 to l2 messages due to flaw in L1KakarotMessaging.sol #36

Closed
c4-bot-10 opened this issue Oct 25, 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-105 edited-by-warden 🤖_primary AI based primary recommendation 🤖_09_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-bot-10
Copy link
Contributor

c4-bot-10 commented Oct 25, 2024

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/solidity_contracts/src/L1L2Messaging/L1KakarotMessaging.sol#L8-L9

Vulnerability details

Proof of Concept

Kakarot’s L1-L2 messaging is based on starknet’s L1-l2 mechanism using the starknet core contract(starknetMessaging).

Based on starknet doc, starknet allows a user to cancel a l1→l2 message to recover funds in case of a failed l2 execution through StarknetMessaging::startL1ToL2MessageCancellation.

The problem is kakarot’s L1KakrotMessaging.sol doesn’t provide any mechanism to invoke startL1ToL2MessageCancellation on starknet core contract. Because kakarot’s users interact with L1KakarotMessaging.sol, this means if l2 tx fails, no cancellation can be initiated on-behalf of the user, and user’s funds will be lost.

//solidity_contracts/src/L1L2Messaging/L1KakarotMessaging.sol
//@audit No method to cancel a l1->l2message (calling StarknetMessaging::startL1ToL2MessageCancellation)
interface IL1KakarotMessaging {
    function sendMessageToL2(address to, uint248 value, bytes memory data) external payable;
    function consumeMessageFromL2(address fromAddress, bytes calldata payload) external;
}

(https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/solidity_contracts/src/L1L2Messaging/L1KakarotMessaging.sol#L8-L9)

L1 sender is allowed to call any L2 evm target contract through L1KakarotMessaging::sendMessageToL2, so the need for L1 user to cancel L1 to L2 message is justified.

POC:
A special case is when kakarot core contract(kakarot.cairo) is paused by admin after L1 user initiated sendMessageToL2 call. L2 tx(handle_l1_message) will fail due to not pausing assertion. L1 user will lose funds and such cases might not be easily prevented.

Impacts:

L1 senders lose funds when Kakarot core is paused after L1-L2 message initiation, or due to other L2 tx failures.

Recommended Mitigation Steps

In L1KakarotMessaging.sol, provide an accounting of user message requests, and add functionality to cancel L1tol2 messages on starknet core contract on behalf of the user.

Assessed type

Other

@c4-bot-10 c4-bot-10 added 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 labels Oct 25, 2024
c4-bot-10 added a commit that referenced this issue Oct 25, 2024
@c4-bot-6 c4-bot-6 changed the title In edge cases, user will lose funds when sending l1 to l2 messages due to flaw in L1KakarotMessaging.sol In edge cases, user might lose funds when sending l1 to l2 messages due to flaw in L1KakarotMessaging.sol Oct 25, 2024
@c4-bot-13 c4-bot-13 added 🤖_09_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels Oct 25, 2024
@howlbot-integration howlbot-integration bot added primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Oct 27, 2024
@ClementWalter
Copy link

Severity: Low

Comment: Only fees would be lost as it is not a bridge. No other funds at risk. But cancellation will be implemented.

@ClementWalter ClementWalter added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Nov 4, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Nov 8, 2024

dmvt marked issue #105 as primary and marked this issue as a duplicate of 105

@c4-judge c4-judge closed this as completed Nov 8, 2024
@c4-judge c4-judge added duplicate-105 satisfactory satisfies C4 submission criteria; eligible for awards and removed primary issue Highest quality submission among a set of duplicates labels Nov 8, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Nov 8, 2024

dmvt marked the issue as satisfactory

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-105 edited-by-warden 🤖_primary AI based primary recommendation 🤖_09_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants