Repository navigation
fix(security): enforce onlyOwner on migration initializers, restore native blocklist modifiers, and guard Celo fee caller - #683
Open
mertcano wants to merge 1 commit into
Conversation
…ative blocklist modifiers, and guard Celo fee caller ### Summary of Changes Addresses **Critical**, **High**, and **Medium** findings from the workspace security audit in `stablecoin-evm`[cite: 27, 32, 37]: 1. **Migration Initializer Access Control (`FiatTokenV2`, `FiatTokenV2_1`, `FiatTokenV2_2` - Critical):** Added `onlyOwner` access control, zero-address validation, and strict version monotonicity checks to prevent unauthorized re-initialization, balance sweeping, and EIP-712 domain corruption[cite: 32, 59, 60, 61]. 2. **Native-Fiat Blocklist Modifier Enforcement (`NativeFiatTokenV2_2` - High):** Restored `notBlacklisted` modifiers across `transfer`, `transferFrom`, and all authorization overloads (`(v, r, s)` and `(bytes signature)`), closing blocklist bypass vectors[cite: 32, 62]. 3. **Celo Fee Caller Zero-Address Guard (`FiatTokenCeloV2_2` - Medium):** Restricted `updateFeeCaller` against `address(0)` to protect the Celo gas lifecycle and added `FeeCallerChanged` event emission for auditability[cite: 37, 58]. 4. **UpgradeabilityProxy Architectural Documentation (Reviewed & Dismissed):** Added explicit architectural notes recording why transparent proxies omit on-chain `proxiableUUID()` checks[cite: 44, 63]. --- ### Vulnerability Analysis - **Unprotected Initializers (Critical - Cat 2):** - `initializeV2`, `initializeV2_1`, and `initializeV2_2` relied strictly on `_initializedVersion` without caller authorization[cite: 32]. - Following proxy upgrade, an attacker could call `initializeV2_1` to drain contract-held USDC to an arbitrary `lostAndFound` address, or invoke `initializeV2_2` to modify the token symbol (corrupting EIP-712 domain separators) and freeze victim accounts[cite: 32]. - **Dropped Blocklist Modifiers (High - Cat 14):** - Overrides in `NativeFiatTokenV2_2` dropped parent `notBlacklisted` modifiers, allowing blocklisted accounts to transfer native-fiat tokens directly or through signed authorizations[cite: 32]. - **Zero-Address Fee Caller (Medium - Cat 8):** - Passing `address(0)` to `updateFeeCaller` permanently broke `debitGasFees`/`creditGasFees`, bricking gas fee payments for Celo transactions[cite: 37, 58]. --- ### Key Remediations - **`contracts/v2/FiatTokenV2.sol`:** - Added `onlyOwner` and `require(initialized && _initializedVersion == 0, ...)` to `initializeV2`[cite: 59]. - **`contracts/v2/FiatTokenV2_1.sol`:** - Added `onlyOwner`, `require(_initializedVersion == 1, ...)`, and `require(lostAndFound != address(0), ...)` to `initializeV2_1`[cite: 60]. - **`contracts/v2/FiatTokenV2_2.sol`:** - Added `onlyOwner` and `require(_initializedVersion == 2, ...)` to `initializeV2_2`[cite: 61]. - **`contracts/v2/NativeFiatTokenV2_2.sol`:** - Restored `notBlacklisted(msg.sender) notBlacklisted(to)` on `transfer`. - Restored `notBlacklisted(from) notBlacklisted(to)` on `transferFrom` and all `transferWithAuthorization` / `receiveWithAuthorization` variants[cite: 62]. - **`contracts/v2/celo/FiatTokenCeloV2_2.sol`:** - Added `require(_newFeeCaller != address(0), "FiatTokenCeloV2_2: zero fee caller")` and emitted `FeeCallerChanged(_newFeeCaller)`. --- ### Verification - Compiled with pinned `solc 0.6.12` via standard-JSON: 0 errors, 0 warnings across all 7 targets (`FiatTokenV2`, `V2_1`, `V2_2`, `NativeFiatTokenV2_2`, `FiatTokenCeloV2_2`, `FiatTokenV1`, `FiatTokenProxy`).
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
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.
Summary of Changes
Addresses Critical, High, and Medium findings from the workspace security audit in
stablecoin-evm[cite: 27, 32, 37]:FiatTokenV2,FiatTokenV2_1,FiatTokenV2_2- Critical): AddedonlyOwneraccess control, zero-address validation, and strict version monotonicity checks to prevent unauthorized re-initialization, balance sweeping, and EIP-712 domain corruption[cite: 32, 59, 60, 61].NativeFiatTokenV2_2- High): RestorednotBlacklistedmodifiers acrosstransfer,transferFrom, and all authorization overloads ((v, r, s)and(bytes signature)), closing blocklist bypass vectors[cite: 32, 62].FiatTokenCeloV2_2- Medium): RestrictedupdateFeeCalleragainstaddress(0)to protect the Celo gas lifecycle and addedFeeCallerChangedevent emission for auditability[cite: 37, 58].proxiableUUID()checks[cite: 44, 63].Vulnerability Analysis
Unprotected Initializers (Critical - Cat 2):
initializeV2,initializeV2_1, andinitializeV2_2relied strictly on_initializedVersionwithout caller authorization[cite: 32].initializeV2_1to drain contract-held USDC to an arbitrarylostAndFoundaddress, or invokeinitializeV2_2to modify the token symbol (corrupting EIP-712 domain separators) and freeze victim accounts[cite: 32].Dropped Blocklist Modifiers (High - Cat 14):
NativeFiatTokenV2_2dropped parentnotBlacklistedmodifiers, allowing blocklisted accounts to transfer native-fiat tokens directly or through signed authorizations[cite: 32].Zero-Address Fee Caller (Medium - Cat 8):
address(0)toupdateFeeCallerpermanently brokedebitGasFees/creditGasFees, bricking gas fee payments for Celo transactions[cite: 37, 58].Key Remediations
contracts/v2/FiatTokenV2.sol:onlyOwnerandrequire(initialized && _initializedVersion == 0, ...)toinitializeV2[cite: 59].contracts/v2/FiatTokenV2_1.sol:onlyOwner,require(_initializedVersion == 1, ...), andrequire(lostAndFound != address(0), ...)toinitializeV2_1[cite: 60].contracts/v2/FiatTokenV2_2.sol:onlyOwnerandrequire(_initializedVersion == 2, ...)toinitializeV2_2[cite: 61].contracts/v2/NativeFiatTokenV2_2.sol:notBlacklisted(msg.sender) notBlacklisted(to)ontransfer.notBlacklisted(from) notBlacklisted(to)ontransferFromand alltransferWithAuthorization/receiveWithAuthorizationvariants[cite: 62].contracts/v2/celo/FiatTokenCeloV2_2.sol:require(_newFeeCaller != address(0), "FiatTokenCeloV2_2: zero fee caller")and emittedFeeCallerChanged(_newFeeCaller).Verification
solc 0.6.12via standard-JSON: 0 errors, 0 warnings across all 7 targets (FiatTokenV2,V2_1,V2_2,NativeFiatTokenV2_2,FiatTokenCeloV2_2,FiatTokenV1,FiatTokenProxy).