WIP Use safe transfers#228
Draft
naddison36 wants to merge 1 commit into
Draft
Conversation
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
Fix unchecked ERC-20 boolean return values across the shared ARM implementation and asset adapters.
This updates ARM and adapter token interactions to use OpenZeppelin
SafeERC20, sotransfer,transferFrom, andapprovecalls that returnfalsenow revert instead of letting execution continue after a failed token operation.Changes
safeTransfer/safeTransferFromfor token movements inAbstractARMforceApprovefor ARM/adaptor approval pathssrc/contracts/adapters/Verification
forge buildforge test --match-path test/unit/OriginARM/ERC20Safety.sol -vvvforge test --match-path test/unit/OriginARM/WrappedOriginAssetAdapter.sol -vvvforge test --match-path test/unit/OriginARM/Deposit.sol -vvvforge test --match-path test/unit/OriginARM/CollectFees.sol -vvvforge testFull suite result: 421 passed, 0 failed, 7 skipped.
Contract Size
Production ARM/adapters remain under the EIP-170 runtime limit, but headroom is tight:
EtherFiARM: 24,550 B, 26 B marginLidoARM: 24,458 B, 118 B marginEthenaARM: 23,935 B, 641 B marginOriginARM: 23,606 B, 970 B marginExisting Asset Behavior
None of the currently configured ARM assets are known to return
falseon failed ERC-20 transfers in normal operation.The mainnet ARM asset set is:
WETH,OETH,wOETHWETH,stETH,wstETHWETH,eETH,weETHUSDe,sUSDeFor the most relevant edge case, canonical mainnet
stETHappears to be true-or-revert:transferandtransferFromcall internal transfer logic and returntrueafter success, while invalid recipients, paused state, insufficient balance, or insufficient allowance revert.So this change is primarily defensive hardening and ERC-20 integration hygiene rather than a fix for a known currently configured asset that returns
false. It prevents future or misconfigured supported assets from minting shares, completing swaps, recording claims, or queueing adapter redemptions after a failed token operation.