audit: zap3 Fixed issues #8
Open
Conversation
This file contains 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
DeFiFoFum
requested changes
Jun 29, 2023
- removeLiquidityGamma: Resolve by transferring in tokens first - Use `estimateSwapReturns` to estimate the amounts for Arrakis LP add - Refactor check for ZapAnalyzer in constructor of Zap - Refactor precision calculation in _getWeightedPrice
… balance left in contract - This helps ensure that users don't send excess Native tokens to the contract they didn't intend on which could be captured by other users - Refactored ApeSwapZapLending to allow for input amount over msg.value
- Organize functions to match similar flow for readability
- Protects against reentrancy attacks during multicall - Protects against reentrancy attacks out of multicall - Provides helper functions for validating checks depending on the state of the multicall
- Organize functions to match similar flow for readability
- Add `receive` function to protect against unwanted native deposits - Rename SwapParams to be more clear - Reset approvals after swap() call
- Fix typography
- Update hardhat-etherscan to version 3
…alidation feat: Add Multicall Guard
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.
ApeSwap-hardhat-zap Paladin audit review
Issue 4: removeLiquidityGamma is flawed
GammaHypervisor requires the removal to be called
fromowner. Why would they do that?https://polygonscan.com/address/0x02203f2351e7ac6ab5051205172d3f772db7d814#code
Issue 23:
The way the liquidity add is calculated on Arrakis differs depending on if there is already liquidity or not: https://github.com/ArrakisFinance/vault-v1-core/blob/7a06f6ffa744eee39b86db9eabb4ea67d3c98aec/contracts/ArrakisVaultV1.sol#L678
estimateSwapReturnsfor ArrakisIssues to revisit
Issue 10: The amount parameter can differ from msg.value
Added check for
msg.value >= amountin case there is a use case where wrap is less than msg.value (don’t know a use case yet)Issue 17:
msg.valueis uncheckednoNativeSurplusmodifier tomulticallto prevent excess BNB from being left in contract.Issues 6, 11, 12, 13, 14, 15, 16, 18, 22, 24, 25, 27, 28
Issues with comments (Resolved)
Issue 1: Token recipients can be address(this)
Fixed for all except ZapLiquidity because LPs can be zapped to pools, vaults etc
Issue 3: Lack of validation for constructor arguments
Fixed except for banana treasury because zero address is allowed if not bnb chain. Validation for bnb chain is in ApeSwapZapPool
Issue 8: Typographical issues
IGammaUniProxy and IGammaHypervisor are used. Renamed UniProxy and Hypervisor interfaces to match file name.
For AddLiquidityV2Params, lpRouter now defined as IV2LiquidityRouter02
Fixed IArrakisRouter lpRouter
Issue 19: GNANA logic is flawed
Fixed by adding check if inputToken == GNANA then transferIn Banana and else just the token like normal.
Discussion (Not resolved)
Issue 2: Unnecessary payable functions
Payable necessary for multicall
Issue 5: Tokens with a fee on transfer will lose value
Aware and noted. Probably not planning to use such tokens here?
Issue 7: Several functions accept native value when they should not
Payable necessary for multicall
Issue 9: The unwrapNative function is flawed
Usage only meant in combination with other functions in multicall. Example: swap busd->wbnb, unwrap and withdraw. Maybe we just shouldn’t allow unwrap and keep in contract. First it could be used for zapping to lending but with changes not anymore.
Issue 20: estimateSwapReturns might run out of gas
No code changes. “Consider monitoring this issue and prepare a custom RPC node that can deal with a custom gas limit.”
Issue 21: Discrepancy between router and factory
We are aware and don’t think it’s a big deal. Just depends on the liquidity type that we need to pass.
Issue 26: getArrakisPool might run out of gas
“Due to the logic in the Arrakis contracts, there is no easy way to solve this issue“. Noted but not sure how much we can do here (and not using Arrakis (yet))
Issue 29: getLPAddRatio for Gamma can be different from result
We have to choose something here. Right in the middle makes the most sense to me. Least chance of reverting because of slippage.
Issue 30: Missing warning for payable modifier
Not added acknowledgment into the code yet. It’s only vulnerable to using msg.value in code which we have in 2 places. wrapNative and ApeSwapZapLending. In both places, you can steal eth if it’s in the contract. But zap is not designed to actually hold any eth and it’s recommended to only do multicalls where ALL funds are used and transferred back. So should not be a (big) issue? But let’s discuss.