Hatch

Issues
1. 🚨 The Hatch app is intended to hold ETH or tokens during the duration of the hatch period. Fund recovery through the built-in escape hatch should be disabled for the contribution token until the sale is successful and close() has been called.
2. 🚨 _refund() should check if the vesting's cliff date has been reached when calculating the amount to burn: because refund() can be called at any time, a user who waits until after the cliff period to be refunded may experience odd behaviour (either they receive some vested tokens or are unable to process the refund because the token manager fails to burn the expected amount of tokens)
  1. Note: vesting is in consideration of being removed
3. ~~🚨 On closing the hatch, is the calculation for the beneficiary's tokens correct? Isn't it the other way around—token.totalSupply() * supplyOfferedPct / (1 - supplyOfferedPct)?~~
  1. Note: The current calculation is correct.
4. 🚨 Avoid using address.transfer() (in _transfer() and _contribute()): these gas-limited methods are now generally seen as an anti-pattern and re-entrancy guards should instead be used when there are concerns with re-entrancy
  - Use .call() with value instead
5. ⚠️ State.Funding state should still be present when _timeSinceOpen() == period (i.e. more correct as _timeSinceOpen() <= period)
6. ⚠️ Does close() need to be guarded by a role? Wouldn't it be better if anyone could call it? Worried that the account who needs to call close() never does.
  - Note: close() is being used by a separate app, to finalize the initial fundraise. Consider changing the functionality in the separate app to read from a closed Hatch than requiring only that app to call close()
7. canPerform() may want to include who is a token holder or other authentication—at the moment if the Hatch app is set as an ACL Oracle, any address will be able to use the granted functionality once the vesting period is complete
  - Note: This was done to allow votes to be (eventually) created even if something catastrophic happened to the Hatch app during fundraising. Consider changing this to be just the initial sale period, rather than full vesting date.
8. contributionToken() is overloaded; either function can be removed or state can be made internal
9. Uses different @aragon/os version than the Augmented Bonding Curve
Notes
1. May be worth upgrading to the hardhat stack, including the @1hive/hardhat-aragon plugin (see this commit as an example)
2. May want to use an explicit type for the reserve storage var—it's not needed here but if Hatch and AugmentedBondingCurve are always used together then it would be better to be explicit
3. Storage slots could be optimized by bundling the uint64s together
4. Initialization checks for vestingCliffPeriod and vestingCompletePeriod can include equality (>=) to allow the cliff to be at the end of the hatch period and the full vested date to be the same as the cliff
5. balanceOf() function is confusing, may be better with a more explicit name like balanceOfInContributionToken()
6. _contribute()'s token branch can be streamlined by removing the balanceOf() and allowance() checks: these are already checked in the token's transferFrom()

Augmented Bonding Curve

Issues
1. 🚨 receiveApproval() should use safeTransferFrom() instead
2. 🚨 _makeBuyOrderRaw() should check length of _buyOrderData
  - I forget for [email protected] whether you need to initialize new memory for this array as well
3. 🚨 Similar to above, avoid using address.transfer() (in _transfer())
4. ⚠️ receiveApproval() may opt to use an additional msg.sender == _token check for clarity
5. ⚠️ _makeBuyOrderRaw() should use the memory keyword for its bytes argument
  - I forget for [email protected] exactly how solidity converts between calldata and memory, but all dynamic types in non-external functions should use memory
Notes
1. May be worth upgrading to the hardhat stack, including the @1hive/hardhat-aragon plugin (see this commit as an example)
2. ApproveAndCall interface is implicitly available but may be better to as an explicitly declared import
3. Collateral struct: group whitelisted with reserveRatio so they can use the same storage slot
4. Are two permissions for buy vs. sell necessary? Any envisioned use case?
5. Similarly, is there a point to including the open() functionality and role? And so, should there be a matching close() functionality and role?
6. Similarly, is it useful to split the collateral-changing roles into three separate roles or could it be left as a single role?
7. _removeCollateralToken() can be simplified to just delete collaterals[_collateral]
8. _makeBuyOrderRaw() does not need an isInitialized check—canPerform() already does this
9. This time, balanceOf() is less confusing, but it may be still better to hide it as an internal function
10. Most roles are important; management is highly sensitive
Additional notes (post 07/09/2021 call)
1. 🚨 makeSellOrder() burns the bond token, decreasing its supply, before calculating the return amount.
  - I'm not 100% confident, but 90%+ confident that the bonding curve contract expects you to calculate a sale with the non-decreased supply so you should be burning the bond token at the end of the operation
  - This is not a problem in the original BatchedBondingCurve contract because batches have a separately cached bond token supply variable.
2. 🚨 _collateralValueIsValid() should not have a case where tokens are pre-deposited by a user—_makeBuyOrder() should always "pull" tokens by calling token.transferFrom()
3. 🚨 _transfer() should not use token.transfer() or change behaviour based on pre-approval state
  - I believe this is from a misunderstanding of how receiveApproval() works with ApproveAndCall tokens. receiveApproval() is simply called immediately after an allowance is given to the target contract, whereby the target contract is allowed to "pull" tokens from the approving user with token.transferFrom().
4. _collateralValueIsValid() can use msg.sender directly instead of accepting it as an argument
5. _collateralValueIsValid() does not need to check user balance or allowance state—this should be done by the token already
6. _makeBuyOrder() would be more efficient if it only pulled the tokens once from a user (collateral tokens + fee) and then distributed fee to beneficiary with a token.transfer().
  - This is because each token.transferFrom() deducts the allowance struct, causing an additional SSTORE.
7. _bondAmountIsValid() does not need to check tokenManager.spendableBalance(), as this is already checked implicitly on transfers. That being said, it can still be a good idea to leave it here as an explicit check.
8. The idea to allow virtual balance and supply to be negative is unfortunately not a reliably safe change in the current design when considering the intended use case of this functionality: to "virtually" inflate bond token balances without affecting the bonding curve. Virtual balance and supply are currently specified per collateral token, allowing each collateral token to use a different bonding curve. However, this design choice means that an inflation in the bond token would require a simultaneous update of all collateral token's virtual balance and supply variables (i.e. through an EVM script). If any collateral token was neglected, a huge arbitrary opportunity would occur. Perhaps a different way to approach this would be to separately track the "spurious" bond tokens by adding a new state variable, and/or adding permissioned functionality in the AugmentedBondingCurve contract to mint these "spurious" tokens without affecting the curve. The collateralSupply calculation could then be adjusted in _makeBuyOrder() and makeSellOrder() to be token.totalSupply().sub(spuriousBondTokenSupply).add(collaterals[_collateral].virtualSupply);.
9. Whitelisted tokens should be ensured to be ERC20 compatible, particularly for balance and allowance guarantees (if a token does not adhere to these standards, there are bigger problems with this token)