Beanstalk3 codehawks remediations #980
Merged
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
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.
Fixes the issues from codehawks.
Final Report:
Codehawks Final Report
High reports:
H-01: LibChainlinkOracle::getTokenPrice will always return instantaneous prices
>0to==0.H-02: LibUsdOracle will compromise Beanstalk peg due to wrong price and DoS.
H-03: LibUsdOracle returns the wrong price for Uniswap Oracle
H-04: ReseedSilo#reseedSiloDeposit does not credit the user any roots
H-05: Successful transactions are not stored, causing a replay attack on redeemDepositsAndInternalBalances
H-06: Internal balances are never actually migrated within L2ContractMigrationFacet
H-07. L2ContractMigrationFacet doesn't increase total Stalk and Roots
H-08. User's stalk is overwritten instead of increased within
ReseedSiloH-09. Grown Stalk is incorrectly calculated in ReseedSilo
H-10. L2ContractMigrationFacet migrates incorrect amount of Stalk
H-11. L2ContractMigrationFacet.addMigratedDepositsToAccount() doesn't update some global balances during the migration.
H-12. Possible loss of user's balances after calling addMigratedDepositsToAccount().
H-13. ReseedSilo doesn't update total balances of Stalk and Roots
H-14. LibPipelineConvert.executePipelineConvert() doesn't decrease Grown Stalk when BDV decreases
H-15. ReseedBarn.sol doesn't initialize of `s.sys.fert.recapitalized
H-16. s.sys.silo.unripeSettings are never set after migration which breaks Unripe functionality
H-19. ReseedSilo does not set the necessary user variables
H-20. Refunding ETH to the caller can be exploited using the Tractor component to call any arbitrary function on behalf of the publisher
H-21. All migrated silo users will receive less stalk accounted to t1heir account during silo reseed
H-22. Incorrect calculation of beanUsdPrice in LibEvaluate::evalPrice
Medium reports:
M-01. USD prices dont work for 20 hours per day
M-02. The declaration and use of LibTractor::BLUEPRINT_TYPE_HASH are inconsistent with the structure struct Blueprint, and the standard is confusing. It is recommended to unify the standard
M-03. SiloFacet::transferDeposit does not verify if amount is 0, leading to full withdrawal DoS for any recipient
M-04. LibUsdOracle is completely broken for the to-deploy L2 chain
M-05. quickSort function does not work as expected, compromising the calculation of Beans per Well to be minted during a flood
M-06. When migrating via L2ContractMigrationFacet, user is not minted roots for the newly accrued stalk
M-07. LibSilo.transferStalk() uses incorrect formula to roundUp
M-08. L2ContractMigrationFacet.addMigratedDepositsToAccount() forfeits "unmowed" rewards from other Silo deposits
M-9. L2ContractMigrationFacet.addMigratedDepositsToAccount() doesn't push depositId to depositIdList
M-10. ReseedField.sol incorrectly configures Field values because of mistake in storage layout
M-11. Invariable.sol won't save Bean from exploit because of flawed entitlement calculation
M-12. Attacker can spam Plots to victim to cause DOS on Plot transfer
M-13. Orderers will lose their Beans after migration to L2
M-16. Improper Domain Separator Hash in _domainSeparatorV4() Function
M-17. Some users would be stuck on the L1 and not be able to migrate their Beans to the L2
08b7789
M-20. Loss/lock of rewards in case of withdrawing fully before a flood
M-21. Stealing SoP rewards by manipulating the total rain roots during converting
M-22. Allowing the execution of more than a single sunrise function when enough time has passed is really dangerous
-Fixed in: Sunrise fixes #985
M-23. If user has not mown since germination, they'll lose their portion of plenty
M-25. redeemDepositsAndInternalBalances should mow before adding migrated deposits
M-27. The function to initialize the whitelisted tokens on L2 will revert due to an out of bound error
M-28. Incorrect Loop Counter Increment in ReseedField Contract
M-29. The default gauge point function is not used even if selector of ss.gaugePointImplementation is 0
M-31. Incorrect conditional in LibUsdOracle leads mal functioning price feeds
Low Reports
L-02. LibUsdOracle inverses Chainlink TWAP price which results in incorrect price
L-07. Cannot configure temperature in ReseedField due to type mismatch
L-08. ReseedBarn.init() will run out of gas due to minting firtilizers on some L2s
L-09. LibWell.getBeanTokenPriceFromTwaReserves() incorrectly assumes that token has 18 decimals
L-10. LibWell.getWellTwaUsdLiquidityFromReserves() returns liquidity with incorrect precision
L-11. LibFertilizer.beginBarnRaiseMigration() incorrectly checks that Oracle supports such token
L-12. SeasonGettersFacet returns the wrong totalDeltaB
L-13. UsdOracle reverts on tokens which are not wstETH, WETH, Bean
L-14. TractorFacet return the wrong values for Tractor Counter
L-15. Zero reward due to a missing scaling factor
L-18. Mowing between two consecutive floods results in loss of rewards related to the second flood
L-19. Malicious users can delete plots from other users in a specific edge case
L-20. If token get's unwhitelisted, users will be unable to claim the plenty they've accrued prior to the unwhitelist
L-23. High Risk Denial-of-Service (DoS) Vulnerability in ERC1155 Token Minting Process.
L-24. Difference with variable decoding between AdvancedPipeCall and AdvancedFarmCal
L-27. Incorrect event emission parameter in TransferBatch event, should use LibTractor._user() instead of msg.sender
L-28. Duplicate Entries in plotIndexes via FieldFacet.sow
Invalid Reports / Risk accepted:
H-18. Unfair Penalty Fees in Pipeline Convert
updateMockPumpUsingWellReserves, which forcefully updates the capped reserves, this doesn't actually happen in a real deployment. Without that line everything works as expected.L-16. Permit functions will not work with certain tokens
L-22. plenty balances are not migrated on L2
L-25. Plot allowances should have defined the index and start
L-26. Permit signatures cannot be cancelled by signers before deadline
permission amount should only be equal to the amount needed for a single desired action. The addition of tractor
allows for a more robust permissioning system and can be used in lieu of the current permit system.
M-15. Forcing penalty to users converting by applying sandwich attack
the advancedFarm calls can be constructed to have any set of criteria implemented to prevent sandwich attacks
from occurring. Thus, adding slippage parameters within the signature itself is not needed.
M-19. Transferring a deposit fully in a rainy season loses the rewards during the flood
deposit in secondary markets (i.e, a user may exchange their deposit for USDC, rather than potentially selling their
beans (which Beanstalk desires when a flood is imminent).
M-18. In transferDeposit the recipient is able to change the stem to a value to take higher grown stalks from the sender
M-19. Transferring a deposit fully in a rainy season loses the rewards during the flood
codebase for marginal gain in utility. Additionally, retaining the flood rewards while transferring allows the potential
flood rewards to be transferred, which may deter users from selling Beans prior to a flood (when Beanstalk would
want beans to be sold in order to return to peg).
M-26. fundsSafu modifier will be useless on L2 before all users have successfully migrated.
glance, depending on how the contract migration is implemented, we may add addtiional validation (such as taking
to account the value of contracts that haven't migrated yet).
H-17. Tokens can get stuck during migration if the L2 side fails leading to loss of funds
L1_EXTERNAL_BEANvariable is incorrectly setduring migration. 2) an attack occurs on the bridge. Given that the DAO will permit the BCM to perform the migration
(and thus no more security assumptions are made) upon the BIP passing, we feel that the potential for 1) to occur is
minimal.
Reports relating to constants / L2 Deployment.
Given that the L2 that beanstalk would migrate to was not known at the time of the contest, many constants that are currently hardcoded in various beanstalk libraries will not work on L2. These reports fall into this catagory.
L-01. The LibWeth hardcodes the WETH address which makes it incompatible on the to-deploy L2 chain
L-03. BeanL1RecieverFacet recieveL1Beans() would never work.
L-04. Incorrect Hardcoded Block Time Assumptions in Beanstalk's LibDibbler
L-05. ETH/USD 1 hour period is too large for Optimism/Base L2 Chains and too small for Arbitrum/Avalanche leading to consuming stale price data.
L-06. The DepotFacet contract uses an incorrect PIPELINE address.
L-17. Mismatch in the BRIDGE address between the BeanL1RecieverFacet and BeanL2MigrationFacet contracts prevents the migration of Beans to L2
L-21. Hardcoded MERKLE_ROOT will make the contract unusable
M-14. Potential Loss of Fertilizer ERC1155 NFTs During L1 to L2 Migration.
Additional Updates:
The following additions were added, that are not remediations. These contain a mix of design upgrades, bug fixes, and QoL changes.
TokenTransferredEvent for ERC20 transfers that occur with Beanstalk's Farm/External balances.