fulldecent commented 2 years ago

Bridgeworld Harvesters contract review

Bridgeworld Harvesters contract review William Entriken Delivered 2022-07-10

Thank you for the opportunity to participate in this review!

Review scope

This is a review of Bridgeworld Harvesters, pre-release version. Code was published at https://github.com/ghoul-sol/treasure-staking/commit/c0840a42dc2c68111ed47f79b8da3f7ce153d6cf with SHA-1 hash c0840a42dc2c68111ed47f79b8da3f7ce153d6cf. Only code in the "harvesters" folder are included in this review, but findings may be included on other topics it they may be helpful.

The scope of review included code-level review (human reading source code), automated code review, (computer reading source code) and unit testing (computer running small-scope tests). Due to limitations of the engagement, the review does not include functional testing (human using compiled program).

The review was performed for the purpose of comparing Bridgeworld Harvesters against the provided "Bridgeworld Harvesters" documentation (accessed 2022-07-08 saved as PDF with SHA-256 hash 5e4a1ce1274154d1d6bdd31aabee215a66c0dcf27375ca4bc71f4c8db6c8101f), the plain English meaning of comments and function names, and a general understanding of NFT games. Any deviation from this standard, or deficiency against relevant best programming practices would be an in-scope review finding.

Sensitivity was set as "anything we should fix or wouldn't want a million people to read".

Review exclusions

Open Zeppelin Contacts were specifically excluded from the scope. Although certain notes about the contracts used are provided below.

MasterOfCoin.sol is explicitly excluded from this review. It appears that TreasuryMine and TreasuryStake are not related to the other files in the project. They are not mentioned in the documentation and they are not connected in any other way to Harvesters and are also excluded.

Quality and correctness of automated testing approaches and deployment strategy is excluded from the scope.

This review does not consider the applicability of this project to any specific jurisdiction's laws or regulations. Although certain of these concepts are discussed below where there is a risk of customer confusion or harm.

Findings

Each finding is listed and is organized by topic. Critical findings that are not resolved are marked with the bomb symbol (💣) and other unresolved material findings are marked with the warning symbol (⚠️). Resolved and less-than-material findings have no emoji marking.

Word choice

Unnecessary use of regulated finance words https://github.com/ghoul-sol/treasure-staking/issues/30

Typos https://github.com/ghoul-sol/treasure-staking/issues/31

Unnecessary use of gendered word for primary controller https://github.com/ghoul-sol/treasure-staking/issues/32

"Boost" word is used inconsistently https://github.com/ghoul-sol/treasure-staking/issues/33

Legion "class" and "rarity" are used without consistent meaning https://github.com/ghoul-sol/treasure-staking/issues/34

_vestedPrincipal function has unexpected functionality https://github.com/ghoul-sol/treasure-staking/issues/35

getHarvesterEmissionsShare function does not return emissions share https://github.com/ghoul-sol/treasure-staking/issues/36

Function isMaxUserGlobalDeposit does not calculate if it is the max https://github.com/ghoul-sol/treasure-staking/issues/37

Dependencies

:warning: Solidity version is unsupported https://github.com/ghoul-sol/treasure-staking/issues/38

:warning: OpenZeppelin Contracts version is unsupported https://github.com/ghoul-sol/treasure-staking/issues/39

Code sniffs and general best practices

:warning: NatSpec documentation is missing https://github.com/ghoul-sol/treasure-staking/issues/40

Revert strings are disguised as revert errors https://github.com/ghoul-sol/treasure-staking/issues/41

HarvestFactory grants all permissions to a single account https://github.com/ghoul-sol/treasure-staking/issues/42

Inconsistent use of option override keyword for implementation of interface https://github.com/ghoul-sol/treasure-staking/issues/43

Design specifications

Conflicting requirements on MAGIC staking utilization multiplier https://github.com/ghoul-sol/treasure-staking/issues/44

Corruption modifier specification is not specific enough https://github.com/ghoul-sol/treasure-staking/issues/45

Legion boost average rank is vulnerable to griefing attack https://github.com/ghoul-sol/treasure-staking/issues/46

Conflicting requirements on timelock periodicity https://github.com/ghoul-sol/treasure-staking/issues/47

Major administrative functions with dire consequences are not listed as "emergency" functions https://github.com/ghoul-sol/treasure-staking/issues/48

Design does not differentiate between fixed and configurable parameters https://github.com/ghoul-sol/treasure-staking/issues/49

Legions boost is missing boost factor https://github.com/ghoul-sol/treasure-staking/issues/50

Design does not specify what happens to disabled harvesters' emissions share https://github.com/ghoul-sol/treasure-staking/issues/51

Specification violations

:bomb: Legion ranks matrix incorrect https://github.com/ghoul-sol/treasure-staking/issues/52

:bomb: Legion weights matrix incorrect https://github.com/ghoul-sol/treasure-staking/issues/53

:bomb: Treasure boosts incorrect https://github.com/ghoul-sol/treasure-staking/issues/54

:bomb: Player stakes cannot be removed if harvester disabled https://github.com/ghoul-sol/treasure-staking/issues/55

:bomb: Extractor boosts are not effective for their full lifetime https://github.com/ghoul-sol/treasure-staking/issues/56

Player can use disallowed locking periods https://github.com/ghoul-sol/treasure-staking/issues/57

AtlasMine does not implement interface detection for super classes https://github.com/ghoul-sol/treasure-staking/issues/58

Rewards duration restriction is not checked https://github.com/ghoul-sol/treasure-staking/issues/59

Implementation issues

:bomb: The liquidity pool can leak liquidity leading to a run on the bank https://github.com/ghoul-sol/treasure-staking/issues/60

:bomb: 1/1 legions can get stuck after setInitialMetadataForLegion https://github.com/ghoul-sol/treasure-staking/issues/61

:bomb: Any NFT can get stuck after _setNftConfig https://github.com/ghoul-sol/treasure-staking/issues/62

:bomb: A configuration error may allow anybody to burn other's NFTs https://github.com/ghoul-sol/treasure-staking/issues/63

Divide-by-zero if admin performs multiple operations at once https://github.com/ghoul-sol/treasure-staking/issues/64

canStake and canUnstake functions are more dangerous than their names imply https://github.com/ghoul-sol/treasure-staking/issues/65

getRatePerSecond produces incorrect result on start of stream https://github.com/ghoul-sol/treasure-staking/issues/66

Testing

Several test cases are not implemented https://github.com/ghoul-sol/treasure-staking/issues/67

Please create test case for Harvester.withdrawPosition with unstaking NFTs in the same transaction https://github.com/ghoul-sol/treasure-staking/issues/68

fulldecent commented 2 years ago

When doing audits I go into "hate everything and find what could go wrong" mode. That's all you're reading in these issues.

But stepping out of that I just want to say this is a nice game design, with care chosen on the setup (21 kg... which isn't a clean divisor of 200 kg... love it!). And I can see a truckload of work went into this, and I can appreciate the compromises that were implemented to make these things work... like using proxies, beacons, configurable options in a lot of places.

As always I appreciate that the Treasure team doesn't commit publicly on ridiculous time lines and really puts a lot of heart into these releases, so keep it up!

I'd love to see this gameplay on a livestream or video sharing websites—will really help fill in the picture for me.

ghoul-sol commented 2 years ago

Thank you for all the issues you reported. It's your 2nd audit I'm working on and I really enjoy your feedback. I can say that it's much more on point and calibrated to be practical than some of the biggest auditors I worked with in another metaverse :)

We will fix a lot of stuff, some will be mitigated and some will be ignored as we have limited resources, unfortunately.

ghoul-sol / treasure-staking