code-423n4 / 2023-01-drips-findings

0 stars 2 forks source link

There is no function to invalidate the nonce #261

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Caller.sol#L164-L183

Vulnerability details

Proof of Concept

The Caller contract is for the implementation of ERC2771Context. Accordingly, it offers 3 main features, which can be mixed and matched ;

The contract has callSigned function to implement EIP712 transactions. So a user's call who is providing a signature can be relayed to the address within the deadline by the protocol after some time. However, if the user wants to cancel it, s/he will not be able to since there is no provision to invalidate this call.

E.g. Alice wants to call function ContractA.b() with 1 ETH Alice signs and sends the signature while providing 11 ETH due to fat finger. Alice wants to cancel this transaction but there is no provision. Surplus 10 ETH is stuck in the Caller contract and used for future gas for the community

Link

Impact

Loss of user funds

Recommended Mitigation Steps

The team might consider implementing a function that cancels this transaction. Many projects solved this by invalidating the nonce;

function invalidateNonce() public {

    nonce[msg.sender]++;
}
GalloDaSballo commented 1 year ago

I believe this to have some validity but perhaps as QA, signer should not use too long of a deadline

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

xmxanuel commented 1 year ago

I think this is a valuable recommendation that we could add. :+1:

I don't know if it qualifies as a bug. Not sure about the servity either. most likely low.

CodeSandwich commented 1 year ago

[disagree with severity: QA] It's very similar to https://github.com/code-423n4/2023-01-drips-findings/issues/63.

I disagree that it's a vulnerability, the signer can disable nonces by signing a dummy transaction with the same nonce and executing it to burn a nonce. I agree that nonces management is a quality of life improvement.

c4-sponsor commented 1 year ago

CodeSandwich marked the issue as disagree with severity

GalloDaSballo commented 1 year ago

Per https://github.com/code-423n4/org/issues/53

Low Severity

L

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-c