Security/envelope hardening#116
Merged
Merged
Conversation
Implements findings from final security review:
H-1: Balance-delta measurement for ERC-20 deposits (fee-on-transfer safety)
- _pullTokensViaApprovalFrom measures balanceOf delta, not requested amount
- Batch functions use actual received / count for per-link amounts
- Raffle links revert with InsufficientTokensReceived for FOT tokens
H-2: Make mfaAuthorizer mutable for key rotation
- Removed immutable, added setMfaAuthorizer(address) onlyOwner
- Emits MfaAuthorizerUpdated event
M-1: Guard _isMfaSignatureValid and _verifyMfaSignature against address(0)
- claimWithMFA reverts with MfaAuthorizerIsZero when authorizer is unset
- Paymaster validation returns false instead of matching address(0)
M-2: Reject unbound links in claimAsBoundRecipient
- Reverts with LinkNotRecipientBound if link.parties.recipient == address(0)
M-3: Reject recipientAddress == address(0) in all claim paths
- _executeClaim reverts with ZeroRecipientAddress
- _isValidClaim returns false for zero recipient (paymaster path)
M-4: Fee-authorization replay protection
- usedFeeAuthorizations mapping tracks consumed signature hashes
- Reverts with FeeAuthorizationAlreadyUsed on replay
L-1: Remove dead DOMAIN_SEPARATOR state and EIP712Domain struct/hash function
L-8: Explicit InvalidContractType revert in _pullUniformBatchAssets
L-9: Remove constructor debug MessageEvent emit
Also adds EnvelopeSecurity.t.sol with 13 targeted tests covering all findings,
updates documentation with security properties section, and fixes spellcheck.
Replace all custom EIP-191 signature schemes with EIP-712: - Inherit OpenZeppelin EIP712 (domain: 'EnvelopeLinks', version '5') - Define CLAIM_TYPEHASH, MFA_APPROVAL_TYPEHASH, FEE_AUTHORIZATION_TYPEHASH - Remove ENVELOPE_SALT constant and assembly-based fee auth digest helpers - All three signature sites now use _hashTypedDataV4(keccak256(abi.encode(...))) Benefits: - Domain separator includes chainId + verifyingContract (cross-chain/contract replay protection) - Wallet-readable signing prompts (EIP-712 structured data) - Compliant with EIP-5267 (eip712Domain() getter) - Removes ~80 lines of hand-rolled assembly for fee auth digest Adds EnvelopeEIP712.t.sol with 19 dedicated tests covering: - Domain separator correctness and uniqueness - Cross-chain and cross-contract replay protection - Typehash verification - Claim mode discrimination - Fuzz tests for arbitrary recipients and deadlines - EIP-5267 getter validation Updates all existing test helpers to use shared EnvelopeEIP712Utils library. All 1072 tests pass. Spellcheck clean.
One attempt was too strict — legitimate retries (wrong gas limit, receiver contract not deployed, token paused then unpaused) would permanently consume the user's gasless quota on the first failure. MAX_GASLESS_ATTEMPTS_PER_LINK = 3 gives users room for honest retries while keeping paymaster sponsorship liability bounded. The test for GaslessAttemptLimitReached now exhausts all 3 allowed attempts before asserting the revert.
…-2, L-6, L-7) Security fixes: - H-1: Reject zero claimKey only when also recipient-bound (allows recipient-only links) - M-1: Constructor + setMfaAuthorizer reject zero authorizer - M-2: Enforce no ETH fees (accumulatedFees scalar, withdrawFees no-arg) - M-3: claimWithMFA bounds-checks index, rejects non-MFA links - M-4: Batch functions early-return on empty list, revert on uneven division - L-1: Fee replay protection uses EIP712 digest - L-2/L-6/L-7: Consolidate signature recovery via _recoverSigner, add NatSpec All 220 envelope tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.