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

No way to cancel l1 -< l2 messages #105

Open
howlbot-integration bot opened this issue Oct 28, 2024 · 5 comments
Open

No way to cancel l1 -< l2 messages #105

howlbot-integration bot opened this issue Oct 28, 2024 · 5 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 M-03 primary issue Highest quality submission among a set of duplicates 🤖_09_group AI based duplicate group recommendation selected for report This submission will be included/highlighted in the audit report 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

@howlbot-integration
Copy link

Lines of code

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

Vulnerability details

Description

There is no api to allow cancellation of l1->l2 messages. In the event of an issue in the kakarot contracts, this will result in the fee being permanently lost since the user has no ability to reclaim the funds.
https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/solidity_contracts/src/L1L2Messaging/L1KakarotMessaging.sol#L26-L61
As we can see there are only functions to either send the message from l1 -> l2 or consume an l2 message. There is no cancelL1toL2Message present.

Recommended Mitigation Steps

https://docs.starknet.io/architecture-and-concepts/network-architecture/messaging-mechanism/#l2-l1_message_cancellation
Introduce the following API to let users cancel their messages after waiting the time limit so that they can reclaim funds.

Assessed type

Context

@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 🤖_09_group AI based duplicate group recommendation bug Something isn't working duplicate-36 sufficient quality report This report is of sufficient quality labels Oct 28, 2024
howlbot-integration bot added a commit that referenced this issue Oct 28, 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 c4-judge reopened this Nov 8, 2024
@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Nov 8, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Nov 8, 2024

dmvt marked the issue as selected for report

@dmvt
Copy link

dmvt commented Nov 8, 2024

Fee loss is still a value leak / loss of funds. Medium is appropriate.

@obatirou
Copy link

As per the doc, see https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#loss-of-fees-as-low

loss of fees is a LOW

@dmvt
Copy link

dmvt commented Nov 16, 2024

Loss of fees should be regarded as an impact similar to any other loss of capital

Context: I helped write this rule as one of the 3 SC judges that agreed on it.

Loss of unmatured yield or yield in motion shall be capped to medium severity.

The intent is that loss of fees is not higher than medium, even if they are substantial yield rewards.

Loss of dust amounts are QA

Only loss of dust is specifically stated as low.

Loss of real amounts depends on specific conditions and likelihood considerations.

The condition above is very likely, making this a valid medium.

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 M-03 primary issue Highest quality submission among a set of duplicates 🤖_09_group AI based duplicate group recommendation selected for report This submission will be included/highlighted in the audit report 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