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

Add protection against gas bombs in prize hooks #115

Merged

Conversation

trmid
Copy link
Member

@trmid trmid commented Jun 26, 2024

Gas bombs in hooks can be triggered in two ways:

  1. returning excessively large data in _hookData for beforeClaimPrize which then gets copied to memory
  2. reverting with an excessively large reason in either beforeClaimPrize or afterClaimPrize hook calls

Gas bombs can be used to drain the remaining call gas and cause entire batch claims to revert, making it difficult for bots to detect, pinpoint, and mitigate the hook failure. To mitigate this behaviour, the ExcessivelySafeCall library is used to limit the return data that is copied to memory.

The prize vault claimPrize function is modified to safely revert if the beforeClaimPrize function returns more than 32 bytes of additional hookData. In addition, if any revert messages are thrown by either hook calls that exceed the total allowed limit of 128 bytes, a generic error will be thrown instead.

By limiting the return data of hook calls, the behaviour of claimPrize is more predictable and claimer bots will not be susceptible gas bombs.

Copy link

linear bot commented Jun 26, 2024

@trmid trmid requested a review from asselstine June 26, 2024 18:26
Copy link

LCOV of commit 8b26879 during coverage #584

Summary coverage rate:
  lines......: 99.6% (246 of 247 lines)
  functions..: 100.0% (66 of 66 functions)
  branches...: no data found

Files changed coverage rate:
                              |Lines       |Functions  |Branches    
  Filename                    |Rate     Num|Rate    Num|Rate     Num
  ==================================================================
  src/abstract/Claimable.sol  | 100%     26| 100%     5|    -      0

Copy link
Contributor

@asselstine asselstine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting case! Looks good, just feels like it might be not enough? Curious to hear your thoughts

/// @dev Revert data for both hooks will also be limited to this size.
/// @dev 128 bytes is enough for `beforeClaimPrize` to return the recipient address as well as 32 bytes of additional
/// byte string data (32 for offset, 32 for length, 32 for data).
uint16 public constant HOOK_DATA_LIMIT = 128;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is three words enough? Do we have more examples to help inform this?

Copy link
Member Author

@trmid trmid Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 words for the entire return data is enough for 32 bytes of _hookData to be passed back from the beforeClaimPrize call. We haven't seen a lot of uses for hook data yet, so it's a pretty arbitrary choice, but it's a way to keep gas costs low while still allowing some data to be passed through. In hindsight, the new transient storage opcodes will probably be used more than this hook data for any meaningful state storage across calls, but the prize vault should still keep this interface intact to be backwards-compatible with previous hooks.

Copy link

LCOV of commit e328b31 during coverage #590

Summary coverage rate:
  lines......: 99.6% (251 of 252 lines)
  functions..: 100.0% (66 of 66 functions)
  branches...: no data found

Files changed coverage rate:
                              |Lines       |Functions  |Branches    
  Filename                    |Rate     Num|Rate    Num|Rate     Num
  ==================================================================
  src/abstract/Claimable.sol  | 100%     26| 100%     5|    -      0

@trmid trmid merged commit 67516cd into fix-review Jun 28, 2024
2 checks passed
@trmid trmid deleted the gen-1778-163-add-protection-against-gas-bombs-in-prize-hooks branch July 15, 2024 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants