An underwriter can pay out multiple times for a single swap failure

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 97Sabit Submission hash (on-chain): 0x3b024e05c1f33a2285524d82f5b2c5c9a679b4ad6e085d7245fc4ea2e601bdf5 Severity: high

Description: Description\ The if (lastTouchBlock != 0) check in the underwrite function implies has been underwritten.

But this is incorrect. It should be checking if (lastTouchBlock == 0) to see if it was not previously underwritten before going on to perform other checks.

Because of this incorrect check, a swap that was previously underwritten in a past block could be underwritten again, leading to potential double underwriting.

The else if check only verifies the swap was not underwritten in the current block. However, because of the incorrect initial check, a swap underwritten in a previous block could still pass this check.

1. Proof of Concept (PoC) File

https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystChainInterface.sol#L739-L754

2. Revised Code File (Optional)

[reednaa]

https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystChainInterface.sol#L729-L731

// Check if the associated underwrite just arrived and has already been matched.
// This is an issue when the swap was JUST underwriten, JUST arrived (and matched), AND someone else JUST underwrote the swap.
// To give the user a bit more protection, we add a buffer of size `BUFFER_BLOCKS`.

The entire block is to ensure that if someone just underwrote a swap, someone else doesn't accidently also underwrite it. IF that is the case, then lastTouchBlock != 0. In that case, we need to do the check.

If lastTouchBlock == 0, then the swap was never underwritten and we don't have to worry about the case mentioned in the beginning of the doc.

Let me know if I misunderstood something.

[ololade97]

Could you also take a look at these comments:

// Get the last touch block. For most underwrites it is going to be 0. uint96 lastTouchBlock = underwritingStorage[identifier].expiry; if (lastTouchBlock != 0) { // implies that the swap has never been underwritten.

[reednaa]

You can argue that the comment might be the opposite but I think the idea comes across very well.

The comment refers to the case when the additional logic is not needed.

[ololade97]

The entire block is to ensure that if someone just underwrote a swap,

Then, lastTouchBlock != 0 doesn't serve as the right check for an underwritten swap.

[reednaa]

That is not the intention either. It is to check if the swap has previously been underwritten because in that case the swap may already have arrived and an underwriter might mistakenly underwrite the swap again.

[ololade97]

It is to check if the swap has previously been underwritten because in that case the swap may already have arrived and an underwriter might mistakenly underwrite the swap again.

This is what I'm trying to say. If the swap has previously been underwritten, it will pass lastTouchBlock != 0 check which is not suppose to be.

[reednaa]

If a swap has previously been underwritten lastTouchBlock != 0. Describe why it would be 0.

[ololade97]

The underwritingStorage mapping stores the state of each underwritten swap, indexed by the swap identifier.

When a new swap is underwritten via the underwrite() function, it generates a unique identifier for that swap based on the swap parameters.

Before allowing the underwriting, it checks underwritingStorage[identifier].lastTouchBlock to see if that swap was recently underwritten.

If lastTouchBlock != 0, it means that swap was already underwritten in the last BUFFER_BLOCKS blocks, so underwriting it again would lead to a double underwriting.

By setting lastTouchBlock = 0 in the if statement, it resets the check so that swap can be underwritten again after BUFFER_BLOCKS blocks have passed.

This prevents two parties underwriting the same swap in a short period of time, which could lead to disputes over who gets the underwriting reward when the swap completes.

The key is that lastTouchBlock != 0 indicates the swap was recently underwritten, so should not be allowed again. Resetting it to 0 makes the swap underwritable again after a buffer period.

[reednaa]

If lastTouchBlock != 0, it means that swap was already underwritten in the last BUFFER_BLOCKS blocks, so underwriting it again would lead to a double underwriting.

Everything you said above this, including this is correct.

By setting lastTouchBlock = 0 in the if statement, it resets the check so that swap can be underwritten again after BUFFER_BLOCKS blocks have passed.

This does not happen.

This prevents two parties underwriting the same swap in a short period of time, which could lead to disputes over who gets the underwriting reward when the swap completes.

This contradicts your previous statement.