address findings from report

[benjaminbollen]

• Attacker can steal tokens from user who opted out consented flow
  • patch01: address path-based groupmint #28 change group mint over path to be minted in group address itself
  • not adopted patch because overreaching: consider restricting operator to only authorized operators for all touched vertices
  • patch 02: remove optout from consented flow to protect users #30 remove opt-out from consented flow
  • acknowledging that upon trusting a group the user is aware that it is the group mint policy that determines whether other people can maximally mint over the user's collateral into the group; or what conditions are enforced
• improve events for wallet UX
  • work within compile size constraint to add event StreamCompleted which is the effective ERC1155:TransferBatch event for each effective batch that is completed in a flow matrix representing a batch of transfer intents as one path transfer
  • replace event DiscountCost with standard ERC1155:TransferSingle(_burner, 0x00, burnAmount) so that standard ERC1155 indexing tracks the correct balances (without specific indexing knowledge of Demurrage "DiscountCost"). Unfortunately we cannot emit both events because of compile size constraints on Hub.sol
  • add events for Group Creation, Mint and Redemption
  • patch 03: improve events #31 see PR for Patch 03
• closed paths constraints can be by-passed, remove it
  • the intention was that an operator can unscatter the distribution of Circles by using closed paths to return Circles to their center where they maximise liquidity. This constraint attempted to block arbitrary moving around of Circles, when a path was "closed" ie. had no explicit senders and receivers; but indeed this constraint is trivially bypassed, so we can remove this constraint.
  • see patch 04: remove closed paths constraint from operateFlowMatrix #32 for code patch
• Base58Converter will fail if alphabet is updated
  • acknowledge that base58 alphabet is defined as a standard and shall not be updated
  • to best of my knowledge the base58 alphabet was introduced by bitcoin and has been widely adopted by many ecosystems, eg
    • https://en.bitcoin.it/wiki/Base58Check_encoding,
    • https://docs.ipfs.tech/concepts/glossary/#multibase
• Public function in Proxy
  • remark: this code is copied from the audited SAFE proxy
  • I have considered EIP1967: reconsider
    • first Gnosis has an older established standard, being part of the Gnosis ecosystem I see no reason not to follow this standard; also SAFE itself has not adopted EIP1967
    • EIP1967 beyond the storage slot "improvement" which is as arbitrary as the first slot (though modern compiler protected), it specifically spends a lot of effort on upgradability and beacon upgradability, which is explicitly not desired. It is not addressed in the EIP how to handle non-upgradable contracts
  • consider code improvements
    • making the storage slot internal is a valid remark and indeed in later versions of SAFE proxy contract this same correction has been adopted; I adopted the changes of SAFE, but your comment that the check for the masterCopy() call is redundant is not true, so I have not adopted that.
  • I have additionally made the proxy not payable as it never needs to receive xDAI in Circles
  • see patch 05: make mastercopy storage slot for Proxy.sol internal #33
• Optimisations and miscellaneous
  • Contract is not IContract
    • Solidity compiler will return non-linear dependency error if adopted (I have fought many times to try to get this, but if you have a solution, please tell me)
  • Additonal SSTORE/SLOAD
    • evaluated and applied, see patch07: remove redundant sstore sload #35
  • Unnecessary re-assignment of values
    • consider applying: while I am pretty certain we never call _update with an empty initialisation of the arrays, this is adapted from the original OpenZeppelin ERC1155 implementation; of course, as a framework they may want to be extra careful as some may use their implementation wrong, so our situation is slightly different. Nonetheless, I feel this is too small an optimisation to venture into more dangerous territory. I would advocate to let this be as it is.
  • Update Math64x64 library
    • updated from explicit copy in 2021 to gitmodule dependency on v3.2 (2023) see patch 08: update math64x64 to v3.2 (2023) #36
• Viewing issuance calculation for groups returns non-zero value, despite actually minting is blocked as designed.
  • add check to return zero for groups and orgs available issuance.
  • see patch 06: make calculateIssuance only return non-zero for Humans #34

[benjaminbollen]

I have merged all patches in order in branch rc-v0.3.5 but the compile size is violated by including patch 07:

the proposed SSTORE/SLOAD optimizations of patch 7 (#35) cause the compile size of Hub.sol to again overshoot (240B from margin of ~90B); so patch7 needs to be evaluated on its merits, and on the merged rc-v0.3.5 we can apply a new patch to undo some of patch 7 to find a compromise between optimising storage reads with the constraints.

(see also same comment on PR itself #35 (comment))

[benjaminbollen]

addresses the compile size constraint in an additional patch #37

[benjaminbollen]

deployed https://github.com/aboutcircles/circles-contracts-v2/releases/tag/rc-v0.3.5-alpha for review; integration; discussion and white hack competition

reference branch of integrated patches under same name: https://github.com/aboutcircles/circles-contracts-v2/tree/rc-v0.3.5