Beanstalk icon indicating copy to clipboard operation
Beanstalk copied to clipboard

Published 20 hours ago • verified publisher icon BeanstalkFarms

Beanstalk3 codehawks remediations

Open Brean0 opened this issue 6 months ago • 0 comments

Fixes the issues from codehawks.

Final Report:

Codehawks Final Report

High reports:

  • H-01: LibChainlinkOracle::getTokenPrice will always return instantaneous prices

    • Solution: Change the ternary check from >0 to ==0.
    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/1011/commits/f0ae1dfeb19d20947aae3f08203f723aed1a2d0e

  • H-02: LibUsdOracle will compromise Beanstalk peg due to wrong price and DoS.

  • H-03: LibUsdOracle returns the wrong price for Uniswap Oracle

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/980/commits/a70bdfca2eb8dbf753049d67d2b2f93c0cf9e38d

  • H-04: ReseedSilo#reseedSiloDeposit does not credit the user any roots

    • Fixed in #999 #1002

  • H-05: Successful transactions are not stored, causing a replay attack on redeemDepositsAndInternalBalances

    • Fixed in #983

  • H-06: Internal balances are never actually migrated within L2ContractMigrationFacet

    • Fixed in #983

  • H-07. L2ContractMigrationFacet doesn't increase total Stalk and Roots

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/983

  • H-08. User's stalk is overwritten instead of increased within ReseedSilo

    • Fixed in #1002

  • H-09. Grown Stalk is incorrectly calculated in ReseedSilo

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/999

  • H-10. L2ContractMigrationFacet migrates incorrect amount of Stalk

    • Fixed in: #999 #1002 #1004

  • H-11. L2ContractMigrationFacet.addMigratedDepositsToAccount() doesn't update some global balances during the migration.

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/983

  • H-12. Possible loss of user's balances after calling addMigratedDepositsToAccount().

    • Fixed in #1002 #999

  • H-13. ReseedSilo doesn't update total balances of Stalk and Roots

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/999

  • H-14. LibPipelineConvert.executePipelineConvert() doesn't decrease Grown Stalk when BDV decreases

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/966
    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/1014

  • H-15. ReseedBarn.sol doesn't initialize of `s.sys.fert.recapitalized

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/999

  • H-16. s.sys.silo.unripeSettings are never set after migration which breaks Unripe functionality

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/999

  • 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

    • Fixed in: #1008

  • H-21. All migrated silo users will receive less stalk accounted to t1heir account during silo reseed

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/1002

  • H-22. Incorrect calculation of beanUsdPrice in LibEvaluate::evalPrice

    • The logic is correct, variable names are wrong.

Medium reports:

  • M-01. USD prices dont work for 20 hours per day

    • Fixed in https://github.com/BeanstalkFarms/Beanstalk/pull/1016

  • 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

    • Fixed:

  • M-03. SiloFacet::transferDeposit does not verify if amount is 0, leading to full withdrawal DoS for any recipient

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/979

  • M-04. LibUsdOracle is completely broken for the to-deploy L2 chain

    • Solution: Remove Chainlink Registry feature.
    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/1015

  • M-05. quickSort function does not work as expected, compromising the calculation of Beans per Well to be minted during a flood

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/980/commits/f4bc9152ec4ab677ed5eb2c8e5f7c541c00dc2da

  • M-06. When migrating via L2ContractMigrationFacet, user is not minted roots for the newly accrued stalk

    • Fixed in: #1002

  • M-07. LibSilo.transferStalk() uses incorrect formula to roundUp

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/997/commits/db009f21274ca3fd9aba1a150a78185e9d78b9ca

  • M-08. L2ContractMigrationFacet.addMigratedDepositsToAccount() forfeits "unmowed" rewards from other Silo deposits

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/983

  • M-9. L2ContractMigrationFacet.addMigratedDepositsToAccount() doesn't push depositId to depositIdList

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/983

  • M-10. ReseedField.sol incorrectly configures Field values because of mistake in storage layout

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/997/commits/78805227297801ade219d3152de13610d40ba212

  • M-11. Invariable.sol won't save Bean from exploit because of flawed entitlement calculation

    • Fixed in:

  • M-12. Attacker can spam Plots to victim to cause DOS on Plot transfer

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/979

  • M-13. Orderers will lose their Beans after migration to L2

    • Fixed in: #1002

  • 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

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/983 https://github.com/BeanstalkFarms/Beanstalk/pull/1011/commits/08b7789226739702688e02e893df138f3cca7ef9

  • M-20. Loss/lock of rewards in case of withdrawing fully before a flood

    • Fixed in ebip17

  • M-21. Stealing SoP rewards by manipulating the total rain roots during converting

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/985

  • M-22. Allowing the execution of more than a single sunrise function when enough time has passed is really dangerous

    -Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/985

  • M-23. If user has not mown since germination, they'll lose their portion of plenty

    • Fixed in Ebip17

  • M-25. redeemDepositsAndInternalBalances should mow before adding migrated deposits

    • Fixed in: #983

  • M-27. The function to initialize the whitelisted tokens on L2 will revert due to an out of bound error

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/983/files#diff-4a3dfd414e5a3956a672ce816084fc50d637eb324c8270c7856ff8fe2b71009fR301-R305

  • M-28. Incorrect Loop Counter Increment in ReseedField Contract

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/commit/e594a417c84a334450710f4896c9f2015c602bc9

  • M-29. The default gauge point function is not used even if selector of ss.gaugePointImplementation is 0

    • Fixed in: #987

  • M-31. Incorrect conditional in LibUsdOracle leads mal functioning price feeds

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/commit/2a44efd040c0722e4a461e6dde85f241a41b69fe

Low Reports

  • L-02. LibUsdOracle inverses Chainlink TWAP price which results in incorrect price

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/991

  • 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

    • Fixed in: #1002

  • L-09. LibWell.getBeanTokenPriceFromTwaReserves() incorrectly assumes that token has 18 decimals

    • need to fix

  • L-10. LibWell.getWellTwaUsdLiquidityFromReserves() returns liquidity with incorrect precision

    • need to fix

  • L-11. LibFertilizer.beginBarnRaiseMigration() incorrectly checks that Oracle supports such token

    • need to fix

  • L-12. SeasonGettersFacet returns the wrong totalDeltaB

    • Fixed in: misc bip, input commit hash here

  • L-13. UsdOracle reverts on tokens which are not wstETH, WETH, Bean

    • UsdOracle is now removed as the usdOracleFacet compasses all features supported by usdOracle.sol

  • L-14. TractorFacet return the wrong values for Tractor Counter

    • Fixed somewhere

  • L-15. Zero reward due to a missing scaling factor

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/984/files#diff-

  • L-18. Mowing between two consecutive floods results in loss of rewards related to the second flood

    • Fixed in ebip17, test added here:

  • L-19. Malicious users can delete plots from other users in a specific edge case

    • Fixed in ebip17

  • L-20. If token get's unwhitelisted, users will be unable to claim the plenty they've accrued prior to the unwhitelist

    • Fixed in:

  • 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

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/commit/c3d00ea3937dd6b593ce341e2424ab6001094685#diff-fbd431ce899b0679e919b2bf1fa828e50eeddd381e4efd2012eca396bb7758daL637

  • L-28. Duplicate Entries in plotIndexes via FieldFacet.sow

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/986/files#diff-41af13e8e4e6d3eac338e5d1777e0d785b938d47d90eb941d0db92e0eb2898aaR91-R92

Invalid Reports / Risk accepted:

  • H-18. Unfair Penalty Fees in Pipeline Convert

    • The auditors test relies on 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

    • If the ERC20 does not use the standard permit, then Beanstalk cannot use these permit systems.

  • L-22. plenty balances are not migrated on L2

    • Beanstalk Farms makes the assumption that the migration will occur before a flood occurs.

  • L-25. Plot allowances should have defined the index and start

    • This is a design choice, as tractor can be used as a substitute for a plot-specific approval for EOAs.

  • L-26. Permit signatures cannot be cancelled by signers before deadline

    • The nonce is not intended to be used as a control system here. In fact, permit system design seems to imply 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

    • sandwich attacks are possible with almost any evm trade that does not have proper slippage tolerances in place. 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

    • This is a design choice, as adding this feature removes fungibility in deposits, and slightly increases the value of the 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

    • Similarly to pods, this is a design choice, as tractor allows for more granular permissioning (such as a specific stem) for a deposit.

  • M-19. Transferring a deposit fully in a rainy season loses the rewards during the flood

    • This is a design choice. Implementing a solution that would allow for this adds significant complexity to the 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.

    • The migration process was updated in the #983 PR. need to re-review this prior to making a judgment. At first 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

    • In the example posted, an error can only occur in 2 ways. 1) The L1_EXTERNAL_BEAN variable is incorrectly set during 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.

    • this will be set upon the BIP passing.

  • L-04. Incorrect Hardcoded Block Time Assumptions in Beanstalk's LibDibbler

    • fixed in https://github.com/BeanstalkFarms/Beanstalk/commit/59da74c5878cb7889bf76c9291087fbb39b428d1

  • 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.

    • https://github.com/BeanstalkFarms/Beanstalk/pull/1016 fixes this by generalizing each oracle with a data parameter (which should contain the timeout).

  • L-06. The DepotFacet contract uses an incorrect PIPELINE address.

    • Fixed in: https://github.com/BeanstalkFarms/Beanstalk/pull/1028

  • L-17. Mismatch in the BRIDGE address between the BeanL1RecieverFacet and BeanL2MigrationFacet contracts prevents the migration of Beans to L2

    • Fixed in: #983

  • L-21. Hardcoded MERKLE_ROOT will make the contract unusable

    • The merkle root will be implemented once the beanstalk 3 BIP has been committed.

  • M-14. Potential Loss of Fertilizer ERC1155 NFTs During L1 to L2 Migration.

    • Smart contract addresses on L1 will need to perform an alternative migration process to the automatic migration process. However, this portion was not implemented correctly in the codebase. Thus, this issue should not occur.
    • Fixed in: #1002

Additional Updates:

The following additions were added, that are not remediations. These contain a mix of design upgrades, bug fixes, and QoL changes.

  1. Update the gauge point function to change based on the distance between the optimal and current deposited bdv.
  • https://github.com/BeanstalkFarms/Beanstalk/pull/1017
  1. Decrease sigma in the morning auction. This decreases the rate in which the morning temperature approaches the max temperature.
  • https://github.com/BeanstalkFarms/Beanstalk/pull/1018
  1. Increase the number of decimals that stalk contains, from 10, to 16.
  • https://github.com/BeanstalkFarms/Beanstalk/pull/1004
  1. Add a TokenTransferred Event for ERC20 transfers that occur with Beanstalk's Farm/External balances.
  • https://github.com/BeanstalkFarms/Beanstalk/pull/1007
  1. Implement an external oracle Implementation which allows beanstalk to query the price of ETH LSD's in USD, using chainlink oracles.
  • https://github.com/BeanstalkFarms/Beanstalk/pull/995
  1. Implement Getter functions to assist in reducing RPC calls necessary to get data in Beanstalk.
  • https://github.com/BeanstalkFarms/Beanstalk/pull/988
  1. Implement reentrancy to support pipeline and depot:
  • #1008
  1. Re-implement original functionality in token facet (i.e allow for external -> internal transfers)
  • #1024

Brean0 avatar Jul 27 '24 15:07 Brean0