| Audit Date | 08/19/2023 - 08/30/2023 |
| Auditor | HollaDieWaldfee (@HollaWaldfee100) |
| Version 1 | 08/30/2023 Initial Report |
| Version 2 | 08/31/2023 Mitigation Review |
- Audit Report - Spire
- Contents
- Disclaimer
- About HollaDieWaldfee
- Scope
- Centralization
- Severity Classification
- Summary
- Findings
- High Risk Findings (2)
- Low Risk Findings (2)
- Improvement Findings (34)
- 5. Use fixed pragma
- 6. Remove unnecessary reentrancy guards
- 7. Make
publicfunctions that are never called internallyexternal - 8. Events can use three indexed parameters
- 9.
ConfigStore.getImplementationUint256: Result can be returned directly - 10. Enforce
overrideconvention when implementing functions from interfaces - 11. Refactor
AccessControl._setAdminContractAccessandAccessControl._setTeamContractAccessfunctions - 12.
ContestStaker: UseRemoveFromSetFailederror instead ofRemoveToSetFailederror - 13. Remove unused errors
- 14.
Contest: Use ofAddresslibrary is not necessary - 15.
Contest.acceptEntries:noWinnermodifier is redundant - 16.
Contest.setWinningEntry:noWinnermodifier is redundant - 17.
Contest.reclaimEntry: UseEntryNotExistsinstead ofNotEntranterror - 18. Optimize gas usage of loops
- 19.
ContestStaker.unstake: Don't delete values in mapping - 20.
ContestStaker.getStakeAmountsForUser: Use existing array to return result - 21.
ContestStaker._removeStakedContractandContestStaker._removeStakeableTokenIdfunctions contain redundant checks - 22. Remove code for "additional echo contests"
- 23. ContestFactory.sol: Set Spire once upon construction
- 24. Simplify
ToggleGovernance.hasEnoughOpenGovernTogglesfunction - 25.
Echoes._mintContestWinner:_teamPercentandteamWalletdon't need to be saved to memory - 26.
Echoes._mintContestWinner: Emit more meaningful value inMintContestRewardevent - 27.
Echoes:_setInitialEchoContestWinnersfunction should be removed - 28. Remove unused imports
- 29. Remove unused events
- 30. Remove unused modifiers
- 31. Variables can be constants
- 32. Function can be declared as
view - 33. Rename
Spire.setEchoContestWinnerstoSpire.setEchoContestWinner - 34.
Echoes:_initialEchoContestsHaveWinnersfunction can be simplified - 35.
Spire.setEchoContestWinnersandSpire.setChapterContestWinner: Gas optimization suggestions - 36.
Spire._advanceEchoFlowIfPossibleandSpire._advanceChapterFlowIfPossible: Gas optimization suggestions - 37.
Spire.approveChapterContestEntriesandSpire.approveEchoContestEntries: Gas optimization suggestions - 38.
Spire:Echo flow and chapter flow are not time-efficient
The following smart contract audit report is based on the information and code provided by the client, and any findings or recommendations are made solely on the basis of this information. While the Auditor has exercised due care and skill in conducting the audit, it cannot be guaranteed that all issues have been identified and that there are no undiscovered errors or vulnerabilities in the code.
Furthermore, this report is not an endorsement or certification of the smart contract, and the Auditor does not assume any responsibility for any losses or damages that may result from the use of the smart contracts, either in their current form or in any modified version thereof.
HollaDieWaldfee is a top-ranked Smart Contract Auditor doing audits on code4rena (www.code4rena.com) and Sherlock (www.sherlock.xyz), having ranked 1st in multiple contests.
On Sherlock he uses the handle "roguereddwarf" to compete in contests.
He can also be booked for conducting Private Audits.
Contact:
Twitter: @HollaWaldfee100
The audit has been conducted in the sponsor's private repository (https://github.com/Hyy-pe/Spire-Contracts) at commit 665b6ab119dd1ded4f388f39d5164823f4735dfb.
The in-scope files are all files in the /src directory.
The mitigation review has been conducted at commit 7ba807b470c7d2e78dfafab2f320e8a35b0591a5.
The protocol must be considered centralized. The MID_ADMIN_ROLE, SUPER_ADMIN_ROLE and owner of the Spire contract must be fully trusted.
The MID_ADMIN_ROLE is able to call the Spire.setURI function (Link) for any tokenId.
Since the ERC1155 tokens, which are minted by the Spire, derive their value from the text / image they represent, changing their URI makes them worthless.
In addition to that, the MID_ADMIN_ROLE is required to choose a winner for every contest.
Without the MID_ADMIN_ROLE's action, the Spire cannot advance.
Also, by not selecting a winner, the frozen tokens for approved submissions cannot be unfrozen.
Therefore by making a submission, the user needs to trust the admins of the protocol that he'll get back his frozen token in case his approved submission is not selected as the winner.
Beyond that, the owner of the ConfigStore contract (which in practice is the same as the owner of the Spire) can change implementation addresses which is another way of messing with the protocol.
What's not subject to centralization risk though are the staked tokens in the ContestStaker contract (Link) which are unfrozen. They are fully controlled by the user that has staked them and it's not possible for a privileged role to access them.
| Severity | Impact: High | Impact: Medium | Impact: Low |
|---|---|---|---|
| Likelihood: High |  |
|
 |
| Likelihood: Medium | |
|
 |
| Likelihood: Low | |
|
|
Impact - the technical, economic and reputation damage of a successful attack
Likelihood - the chance that a particular vulnerability is discovered and exploited
| Severity | Total | Fixed | Acknowledged | Disputed | Reported |
|---|---|---|---|---|---|
|
2 | 2 | 0 | 0 | 0 |
|
0 | 0 | 0 | 0 | 0 |
|
2 | 2 | 0 | 0 | 0 |
|
34 | 34 | 0 | 0 | 0 |
Description:
Once a sufficient amount of entries has been submitted to a Contest and the minimumContestTime has passed, the Contest is closed. (Link)
At this point no further entries can be submitted. (Link)
This introduces the possibility of a griefing attack.
Impact:
If all entrant addresses are contracts that revert when they get the reward minted and the onERC1155Received callback is executed, it is not possible to select a winner.
Thereby the Spire is halted forever.
Determining the severity of this finding is not straightforward because it relies on the sophistication that we ascribe to the MID_LEVEL_ADMIN who is responsible for approving entries.
Deciding whether an entry is malicious is non-trivial. It requires on-chain analysis and in-depth knowledge of the EVM.
Adding to that the severe consequences, i.e. halting the operation of the Spire forever, I determine this issue to be of High severity.
The following test can be used to verify the issue:
/* @audit-info
add to Echoes.t.sol
add import: import "forge-std/console.sol";
import KillSwitch contract
*/
function test_setInitialEchoContestWinners_HollaDieWaldfee_8() public {
KillSwitch killSwitch = new KillSwitch();
address payable[] memory entrant = new address payable[] (1);
entrant[0] = payable(address(killSwitch));
echoesHarness.createInitialEchoContests(0, 0, 3600, 4);
// Sets winners on contests that have entries and can be closed.
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT - 1; i++) {
Contest echoContest = Contest(echoesHarness.chapterEchoes(0, 0, i));
assertFalse(echoContest.hasWinner());
contestUtils.submitAndAcceptEntries(entrant, echoContest, 4, 1);
}
utils.mineTime(3600);
/* @audit-info
setting winnner for echo contest 0
*/
echoesHarness.setInitialEchoContestWinners(0, 0, 0, 0, new uint256[](0));
/* @audit-info
make entrant revert upon being minted the reward token
*/
killSwitch.setKillSwitch(true);
vm.expectRevert();
echoesHarness.setInitialEchoContestWinners(0, 0, 1, 0, new uint256[](0));
}To execute the test, the KillSwitch contract is needed:
pragma solidity ^0.8.0;
contract KillSwitch {
bool public killSwitch = false;
function setKillSwitch(bool _killSwitch) external {
killSwitch = _killSwitch;
}
function onERC1155Received(
address,
address,
uint256,
uint256,
bytes memory
) external view returns (bytes4) {
if (killSwitch) {
return bytes4(0x00000000);
}
return this.onERC1155Received.selector;
}
}Recommendation:
The Spire.setEchoContestWinners, Spire.setChapterContestWinner and Spire.setWinningEntryForAdditionalEchoContest (as I argue in another finding, this third function should be removed) functions should be changed to have an additional bool toBeneficiary parameter that is passed along until the Echoes._mintContestWinner function is reached.
If toBeneficiary = true, the reward for the winner should be minted to the beneficiary instead.
Thereby it's possible for the MID_LEVEL_ADMIN to resolve any situation where the ERC1155._mint call reverts.
Changes to the Echoes._mintContestWinner function:
diff --git a/src/Echoes.sol b/src/Echoes.sol
index d14833d..2159ac5 100644
--- a/src/Echoes.sol
+++ b/src/Echoes.sol
@@ -67,12 +67,16 @@ contract Echoes is HasConfigStore, ERC1155Supply {
emit URI(uri, tokenId);
}
- function _mintContestWinner(address winner) internal returns (uint256 contestRewardTokenId) {
+ function _mintContestWinner(address winner, bool toBeneficiary) internal returns (uint256 contestRewardTokenId) {
contestRewardTokenId = nextContestWinnerId++;
uint256 _teamPercent = _getTeamPercent();
uint256 _teamCount = GlobalConstants.CONTEST_REWARD_AMOUNT * _teamPercent / (100);
address _teamWallet = _getTeamWallet();
- _mint(winner, contestRewardTokenId, GlobalConstants.CONTEST_REWARD_AMOUNT - _teamCount, "");
+ if (toBeneficiary) {
+ _mint(_getBeneficiary(), contestRewardTokenId, GlobalConstants.CONTEST_REWARD_AMOUNT - _teamCount, "");
+ } else {
+ _mint(winner, contestRewardTokenId, GlobalConstants.CONTEST_REWARD_AMOUNT - _teamCount, "");
+ }
_mint(_teamWallet, contestRewardTokenId, _teamCount, "");
emit MintContestReward(contestRewardTokenId, winner, GlobalConstants.CONTEST_REWARD_AMOUNT);
}All other changes only involve passing around the toBeneficiary parameter.
Resolution:
The recommendation has been implemented and the toBeneficiary parameter is now passed from the Spire.setChapterContestWinner and
Spire.setEchoContestWinner functions down to the Echoes._mintContestWinner function.
The beneficiary address is a trusted address which is known to accept ERC1155 tokens.
2. Griefing attack to halt the Spire caused by insufficient access control around Contest.reclaimEntry function 
Description:
The Contest.reclaimEntry function allows to unfreeze the staked token once a winner has been selected. (Link)
The function can be called by anyone for any entry (except the winning entry).
By unfreezing a token multiple times, the operation of another Contest can be impacted, i.e. it may not be able to select a winner.
Impact:
The severity of this finding is High.
When a Contest is not able to select a winner, the Spire is halted forever.
It is not possible for the MID_LEVEL_ADMIN to detect this attack if it is executed by a determined attacker (using multiple addresses, high quality entries).
The following test can be used to verify the issue:
/* @audit-info
add to Contest.t.sol
add import: import "forge-std/console.sol";
*/
function test_reclaimEntryHollaDieWaldfee() public {
address payable[] memory entrant = utils.createUsers(1);
uint256[] memory entryIdsContest1 = contestUtils.submitAndAcceptEntries(entrant, contest, 8, 0);
/* @audit-info
Setting up another contest.
This is the contest that will be DOS'd
*/
vm.prank(contestAdmin);
Contest contest2 = Contest(contestFactory.deployNewContest(7 days, 8, configStore));
uint256[] memory entryIdsContest2 = contestUtils.submitAndAcceptEntries(entrant, contest2, 8, 0);
utils.mineTime(7 days);
/* @audit-info
frozen stakes for the user at this point are 16.
Both contests have 8 entries, each entry freezinh 1 token
*/
console.log(contestStaker.frozenStakes(address(stakedToken), 0, entrant[0]));
/* @audit-info
winner for contest 1 is selected such that the reclaimEntry function can be called
*/
vm.prank(contest.owner());
contest.setWinningEntry(entryIdsContest1[0]);
/* @audit-info
frozen stakes of the user are set to zero by repeatedly calling reclaimEntry()
*/
while (contestStaker.frozenStakes(address(stakedToken), 0, entrant[0]) > 0) {
contest.reclaimEntry(entryIdsContest1[1]);
console.log(contestStaker.frozenStakes(address(stakedToken), 0, entrant[0]));
}
/* @audit-info
contest 2 is closed
since all approved entries in the second contest belong to the user,
and the user's frozen stakes are 0, no winning entry can be selected
reverts here:
https://github.com/Hyy-pe/Spire-Contracts/blob/665b6ab119dd1ded4f388f39d5164823f4735dfb/src/ContestStaker.sol#L157-L159
*/
for (uint i = 0; i < entryIdsContest2.length; i++) {
vm.prank(contest2.owner());
vm.expectRevert(); // reverts due to insufficient stake
contest2.setWinningEntry(entryIdsContest2[i]);
}
/* @audit-info
just to illustrate a possible solution that the Spire admins might come up with
there's no way to recover from this situation without the help of the malicious entrant
he can hold the Spire hostage
*/
vm.expectRevert();
contestUtils.submitAndAcceptEntries(entrant, contest2, 1, 0);
}Recommendation:
As discussed with the client, the recommendation is to add the onlyOwner modifier to the Contest.reclaimEntry function.
This ensures that it's only possible for the Spire contract to reclaim entries.
The entries that need to be reclaimed are known at the time the winner is selected and their IDs need to be provided by the MID_LEVEL_ADMIN when calling the Spire.setChapterContestWinner and Spire.setEchoContestWinners function (there's also the Spire.setWinningEntryForAdditionalEchoContest function but, as I discuss in another finding, this function should be removed).
An additional layer of safety is to add the if (!entries[entryId].isApproved) revert EntryNotApproved(); check to the Contest.reclaimEntry function.
Thereby the MID_LEVEL_ADMIN cannot reclaim an approved entry which ensures it can't be reclaimed / cancelled twice by calling Contest.cancelEntry as well.
Resolution:
The fix has been implemented as recommended. The onlyOwner modifier has been added to the Contest.reclaimEntry function and the additional check that the entry must be approved has been added as well.
For approved entries, they are "cancelled" by calling Contest.reclaimEntry which can only happen once as part of setting a winner.
On the other hand, unapproved entries are cancelled by calling Contest.cancelEntry which deletes the entry once it's been cancelled and thereby only allows for one cancellation.
In summary it's now ensured that cancellation / reclaiming only happens once.
Also, no other changes to the Contest contract conflict with this fix.
Description:
The chapter flow must be initialized by the MID_LEVEL_ADMIN by calling Spire.initializeChapterFlow (Link).
It is not ensured that this function is only called once because the check does not cover all ways in which the chapter flow could have been initialized.
Thereby the chapter flow can reach an inconsistent state, leaving it unable to close / set a winner for a chapter contest which means the affected genesis text id becomes stale and cannot advance.
Impact:
The following test can be added to the Spire.t.sol file.
It illustrates how the chapter flow can be initialized twice. First at chapter 0 of genesis text 1, then at chapter 0 of genesis text 2.
The severity of the finding is Low because it requires a mistake made by the MID_LEVEL_ADMIN role which is trusted.
function test_initializeChapterFlowTwice() public {
uint256 winningId = spireUtils.completeGenesisEchoes(0);
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(0, winningId, i, new uint256[](0));
}
/* @audit-info
set winners for echo contests in genesis text id 1
*/
winningId = spireUtils.completeGenesisEchoes(1);
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(1, winningId, i, new uint256[](0));
}
console.log(spire.previousChapterFlowId());
console.log(spire.currentChapterFlowId());
/* @audit-info
chapter flow starts at genesis text id 1
*/
ToggleGovernance tg = ToggleGovernance(spire.genesisGovernors(0));
vm.startPrank(spire.owner());
spire.setApprovalForAll(address(tg), true);
tg.stake(GlobalConstants.MAX_CHAPTER_COUNT);
spire.initializeChapterFlow();
vm.stopPrank();
console.log(spire.previousChapterFlowId());
console.log(spire.currentChapterFlowId());
/* @audit-info
set winners for echo contests in genesis text id 2
*/
winningId = spireUtils.completeGenesisEchoes(2);
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(2, winningId, i, new uint256[](0));
}
/* @audit-info
can initialize chapter flow again at genesis text id 2
*/
tg = ToggleGovernance(spire.genesisGovernors(1));
vm.startPrank(spire.owner());
spire.setApprovalForAll(address(tg), true);
tg.stake(GlobalConstants.MAX_CHAPTER_COUNT);
spire.initializeChapterFlow();
vm.stopPrank();
console.log(spire.previousChapterFlowId());
console.log(spire.currentChapterFlowId());
}Recommendation:
Check the value of the _initializeChapterFlow variable instead of whether the chapter contest 0 at genesis text 0 exists.
diff --git a/src/Spire.sol b/src/Spire.sol
index cba361f..eecefb2 100644
--- a/src/Spire.sol
+++ b/src/Spire.sol
@@ -370,7 +370,7 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
// check whether the first chapter was created everytime we set an Echo chapter contest winner. We only need to
// kickstart the chapter flow once, so requiring it to be manually called once seems OK.
function initializeChapterFlow() external nonReentrant {
- if (_hasCreatedChapterContest(0, 0)) revert ChapterFlowInitialized();
+ if (_initializeChapterFlow) revert ChapterFlowInitialized();
while (_shouldSkipGenesisText(currentChapterFlowId, Flow.Chapter)) {
if (currentChapterFlowId + 1 < GlobalConstants.GENESIS_TEXT_COUNT) {Resolution:
The recommendation has been implemented.
Description:
When an entry for a contest is submitted via the Contest.submitEntry function (Link) it can be cancelled as long as it's not approved via the Contest.cancelEntry function (Link).
Entries are indexed by an id that is determined by the user when the entry is created.
Different entries can have the same id if one entry is created, cancelled and then then the other entry with the same id is created.
This introduces race conditions between users and between users and the Spire.
Impact:
There are two basic scenarios.
Scenario 1:
- User 1 wants to submit an entry with id x
- User 2 front-runs User 1 and submits an entry with the same id, then cancels it after User 1's transaction has failed
- This can go on multiple times such as to actually impact the availability of the
Contest
Scenario 2:
- User 1 has submitted an entry with id x
MID_LEVEL_ADMINdecides to approve it- User 1 front runs
MID_LEVEL_ADMINby cancelling the entry and submitting a new entry with another URI - an unintended URI is approved (This can lead to problems in later iterations of the protocol where approved but losing entries have further abilities. Currently this does not pose a significant risk because the
MID_LEVEL_ADMINcan always just set the URI to any value manually)
The first Spire will be deployed on Arbitrum, which does not have a mempool.
Thereby the risk of front-running is mitigated.
However it has been stated by the client that the protocol should be chain-agnostic.
Recommendation:
It is recommended to implement a counter for the entry ids which gets incremented whenever an entry is created.
This ensures that no two entries have the same id and submitting an entry cannot be front-run.
Resolution:
The counter has been implemented and the Contest.submitEntry function now returns the id of the entry that is created.
Description:
All the Solidity files specify the Solidity version as ^0.8.0.
It is recommended to use a fixed Solidity version to avoid compilation with an unintended version.
The version that is specified in the Foundry configuration file is 0.8.17, so this version should be specified in the Solidity files as well.
Resolution:
The recommendation has been implemented, all Solidity files now use 0.8.17 as the Solidity version.
Description:
While reentrancy guards offer protection against reentrancy attacks, they introduce some overhead.
I recommend to remove the reentrancy guards in the following instances because there's obviously no way to reenter the function, i.e. there is no call to a component outside the protocol that could reenter.
Also consider that by removing reentrancy guards, some contracts may no longer need to inherit from ReentrancyGuard.
Resolution:
The recommendation has been implemented.
Description:
This improves readability and can save some gas due to how parameters for public and external functions are treated differently.
Reference:
https://gus-tavo-guim.medium.com/public-vs-external-functions-in-solidity-b46bcf0ba3ac
Resolution:
The recommendation has been implemented.
Description:
It is recommended to max out the number of indexed parameters for all events even if it's not considered necessary to make the parameters indexed.
Resolution:
The recommendation has been implemented.
Description:
In the ConfigStore.getImplementationUint256 function, the return value is saved to memory before it is returned. (Link)
Recommendation:
Return the result directly, without saving it to memory:
diff --git a/src/ConfigStore.sol b/src/ConfigStore.sol
index 15f9c0e..af092ad 100644
--- a/src/ConfigStore.sol
+++ b/src/ConfigStore.sol
@@ -54,7 +54,6 @@ contract ConfigStore is IConfigStore, Ownable, Multicall {
*/
function getImplementationUint256(bytes32 interfaceName) external view returns (uint256) {
address implementationAddress = interfacesImplemented[interfaceName];
- uint256 implementationUint256 = uint256(uint160(implementationAddress));
- return implementationUint256;
+ return uint256(uint160(implementationAddress));
}
}Resolution:
The recommendation has been implemented.
Description:
In the codebase, when a function from an interface is implemented, that function is marked with the override modifier.
I recommend to enforce this convention and to add the override modifier to where it is missing.
Resolution:
The recommendation has been implemented.
11. Refactor AccessControl._setAdminContractAccess and AccessControl._setTeamContractAccess functions 
Description:
The functions contain an unnecessary check for _revokedAdminAccess and _revokedTeamAccess respectively.
Both checks can only be reached when the respective variable is false, therefore its value does not have to be checked.
Recommendation:
diff --git a/src/AccessControl.sol b/src/AccessControl.sol
index 9a282b9..992c7ef 100644
--- a/src/AccessControl.sol
+++ b/src/AccessControl.sol
@@ -282,13 +282,13 @@ abstract contract AccessControl is Context, IAccessControl {
function _setAdminContractAccess(bool access) internal {
if (_revokedAdminAccess) revert("AccessControl: access denied");
- _revokedAdminAccess = !_revokedAdminAccess ? !access : true;
+ _revokedAdminAccess = !access;
_hasAdminAccess = access;
}
function _setTeamContractAccess(bool access) internal {
if (_revokedTeamAccess) revert("AccessControl: access denied");
- _revokedTeamAccess = !_revokedTeamAccess ? !access : true;
+ _revokedTeamAccess = !access;
_hasTeamAccess = access;
}
}Resolution:
The recommendation has been implemented.
Description:
It makes no sense to speak of "remove to a set".
The correct RemoveFromSetFailed error to replace it already exists but it's never used.
I recommend to replace the instances of RemoveToSetFailed with RemoveFromSetFailed and to remove the RemoveToSetFailed error from the contract entirely.
Recommendation:
diff --git a/src/ContestStaker.sol b/src/ContestStaker.sol
index fbef5c2..fc23d18 100644
--- a/src/ContestStaker.sol
+++ b/src/ContestStaker.sol
@@ -19,7 +19,6 @@ import { ReentrancyGuard } from "openzeppelin-contracts/contracts/security/Reent
error UnregisteredContest();
error AddToSetFailed();
error RemoveFromSetFailed();
-error RemoveToSetFailed();
error StakedContractNotExists();
error TokenIdNotExists();
error InvalidStakedContract();
@@ -266,7 +265,7 @@ contract ContestStaker is IContestStaker, ERC1155Holder, ReentrancyGuard, Multic
uint256 len = tokenIds.length;
for (uint256 i = 0; i < len; i++) {
bool success = stakeableTokenIds[stakedContract].remove(tokenIds[i]);
- if (!success) revert RemoveToSetFailed();
+ if (!success) revert RemoveFromSetFailed();
emit RemovedTokenId(stakedContract, tokenIds[i]);
}
emit RemovedStakedContract(stakedContract);
@@ -278,7 +277,7 @@ contract ContestStaker is IContestStaker, ERC1155Holder, ReentrancyGuard, Multic
revert TokenIdNotExists();
}
bool success = stakeableTokenIds[stakedContract].remove(tokenId);
- if (!success) revert RemoveToSetFailed();
+ if (!success) revert RemoveFromSetFailed();
emit RemovedTokenId(stakedContract, tokenId);
}
}Resolution:
The recommendation has been implemented.
Resolution:
The recommendation has been implemented.
Description:
There's no use of a function from the Address library.
Hence it is recommended to remove the following two lines:
Resolution:
The recommendation has been implemented.
Description:
The noWinner modifier is redundant because there's also the check that the Contest must not be closed:
function acceptEntries(uint256[] memory entryIds) external override onlyOwner noWinner nonReentrant {
if (isClosed()) revert Closed();
uint256 newlyAcceptedEntries;
...In all cases when there is a winner and noWinner reverts, the Contest is also closed.
Therefore I recommend to remove the noWinner modifier.
Note: Removing isClosed() instead of noWinner is not possible!
Resolution:
The recommendation has been implemented.
Description:
The Contest.setWinningEntry function has the noWinner modifier and also checks that winner.winner must be address(0). (Link)
Both are the same check, therefore it is recommended to remove one instance of the check.
Resolution:
The recommendation has been implemented.
Description:
The wrong error is used. The error that correctly describes the problem is EntryNotExists. (Link)
Resolution:
The recommendation has been implemented.
Description:
There are many instances of for-loops that can be optimized for Gas usage.
Clearly the client is aware of how to optimize for-loops for gas usage as can be seen in this instance.
There's just one additional improvement: Don't initialize the counter variable with 0.
echoCount[genesisTextId][chapterId] is checked to be 0, so i can just be defined as uint i wihout initialization.
https://github.com/Hyy-pe/Spire-Contracts/blob/665b6ab119dd1ded4f388f39d5164823f4735dfb/src/Echoes.sol#L101-L103
Resolution:
The for-loops have been optimized for gas usage.
For some instances, further improvements could be made which were implemented in commit da65e29b6d637bb6a11999bf968c9e41538cab50 and commit b6d8fe4649fade25a16f7af9fb3fe80e6a66c0c9.
Description:
In the ContestStaker.unstake function, the delete keyword is used to clear the value in the stakes and frozenStakes mapping. (Link)
In case this code executes, both entries already have the value 0, so it's not necessary to clear them again.
The maximum gas consumption of the unstake function can be reduced by almost 800 Gas, according to the forge gas report.
Recommendation:
Remove the redundant code.
diff --git a/src/ContestStaker.sol b/src/ContestStaker.sol
index fbef5c2..cee62e7 100644
--- a/src/ContestStaker.sol
+++ b/src/ContestStaker.sol
@@ -191,11 +191,7 @@ contract ContestStaker is IContestStaker, ERC1155Holder, ReentrancyGuard, Multic
}
if (amount == 0) revert InvalidInputAmount();
stakes[stakedContract][tokenId][msg.sender] -= amount;
- // If stake is now 0, delete the entries to give caller gas refund.
- if (stakes[stakedContract][tokenId][msg.sender] == 0) {
- delete stakes[stakedContract][tokenId][msg.sender];
- delete frozenStakes[stakedContract][tokenId][msg.sender];
- }
+
IERC1155(address(stakedContract)).safeTransferFrom(address(this), msg.sender, tokenId, amount, "");
emit Unstaked(stakedContract, tokenId, amount, msg.sender);
}Resolution:
The recommendation has been implemented.
Description:
In addition to optimizing the gas-usage of the loop, it is not necessary to create a second array to store the result.
The result can simply be written back into the existing allTokenIds array.
Recommendation:
diff --git a/src/ContestStaker.sol b/src/ContestStaker.sol
index fbef5c2..6dfc999 100644
--- a/src/ContestStaker.sol
+++ b/src/ContestStaker.sol
@@ -219,13 +219,13 @@ contract ContestStaker is IContestStaker, ERC1155Holder, ReentrancyGuard, Multic
)
public
view
- returns (uint256[] memory tokenIds)
+ returns (uint256[] memory)
{
uint256[] memory allTokenIds = getStakeableTokenIds(stakedContract);
- tokenIds = new uint256[](allTokenIds.length);
for (uint256 i = 0; i < allTokenIds.length; i++) {
- tokenIds[i] = getUsableStake(stakedContract, allTokenIds[i], user);
+ allTokenIds[i] = getUsableStake(stakedContract, allTokenIds[i], user);
}
+ return allTokenIds;
}Resolution:
The recommendation has been implemented.
21. ContestStaker._removeStakedContract and ContestStaker._removeStakeableTokenId functions contain redundant checks
Description:
The remove function from the EnumerableSet library only returns false if the value to be removed does not exist.
Since in both functions (ContestStaker._removeStakedContract and ContestStaker._removeStakeableTokenId) it is ensured that the value does in fact exist in the set, it is unnecessary to check the return value of remove.
Recommendation:
Remove the redundant checks.
diff --git a/src/ContestStaker.sol b/src/ContestStaker.sol
index fbef5c2..9b850ab 100644
--- a/src/ContestStaker.sol
+++ b/src/ContestStaker.sol
@@ -265,9 +265,7 @@ contract ContestStaker is IContestStaker, ERC1155Holder, ReentrancyGuard, Multic
uint256[] memory tokenIds = stakeableTokenIds[stakedContract].values();
uint256 len = tokenIds.length;
for (uint256 i = 0; i < len; i++) {
- bool success = stakeableTokenIds[stakedContract].remove(tokenIds[i]);
- if (!success) revert RemoveToSetFailed();
- emit RemovedTokenId(stakedContract, tokenIds[i]);
+ stakeableTokenIds[stakedContract].remove(tokenIds[i]);
}
emit RemovedStakedContract(stakedContract);
}
@@ -277,8 +275,7 @@ contract ContestStaker is IContestStaker, ERC1155Holder, ReentrancyGuard, Multic
if (!stakedContracts[stakedContract] || !stakeableTokenIds[stakedContract].contains(tokenId)) {
revert TokenIdNotExists();
}
- bool success = stakeableTokenIds[stakedContract].remove(tokenId);
- if (!success) revert RemoveToSetFailed();
+ stakeableTokenIds[stakedContract].remove(tokenId);
emit RemovedTokenId(stakedContract, tokenId);
}
}Resolution:
The recommendation has been implemented after further discussion in commit ab56b1a036a7d21a79506591f2abedc68ac5dd95.
Description:
"Additional echo contests" are echo contests beyond the initial five echo contests for each chapter.
As discussed with the client, these additional contests are not supported in this iteration of the protocol and it's not clear in which form - if at all - they will be supported in the future.
Therefore the unused code should be removed.
Note that there is no security impact to the existence of this half-baked functionality, as the affected functions can only be called by a trusted MID_LEVEL_ADMIN or beneficiary.
Code to remove:
https://github.com/Hyy-pe/Spire-Contracts/blob/665b6ab119dd1ded4f388f39d5164823f4735dfb/src/Spire.sol#L526-L577
Resolution:
The recommendation has been implemented.
Description:
The ContestFactory contract has a setSpire function.
It is sufficient to set the Spire address once upon construction, it does never change. (Link)
Recommendation:
I propose the following changes to the ContestFactory contract:
diff --git a/src/ContestFactory.sol b/src/ContestFactory.sol
index 963bfa7..c7f03a7 100644
--- a/src/ContestFactory.sol
+++ b/src/ContestFactory.sol
@@ -38,10 +38,13 @@ contract ContestFactory is IContestFactory, ContestStaker, Ownable {
constructor(
address _stakedContract,
- uint256[] memory _stakeableTokenIds
+ uint256[] memory _stakeableTokenIds,
+ address spire
)
ContestStaker(_stakedContract, _stakeableTokenIds) // solhint-disable-next-line no-empty-blocks
- { }
+ {
+ _spire = spire;
+ }
function addStakeableTokenId(address stakedContract, uint256 tokenId) public onlyContract nonReentrant {
_addStakeableTokenId(stakedContract, tokenId);
@@ -55,12 +58,6 @@ contract ContestFactory is IContestFactory, ContestStaker, Ownable {
_removeStakeableTokenId(stakedContract, tokenId);
}
- function setSpire(address spire) public onlyOwner {
- if (spire == address(0)) revert InvalidSpireAddress();
- _spire = spire;
- emit SetSpireAddress(_spire);
- }
-
// Anyone can call this function to deploy a new Contest that sets the caller as the owner of the new Contest.
// This should be called by the Spire contract but there is no harm if anyone calls it.
function deployNewContest(The deployment order of the protocol must be changed as well, here are the changes that need to be made to CommonTestBase.t.sol:
diff --git a/test/CommonTestBase.t.sol b/test/CommonTestBase.t.sol
index 604f1de..5088ab2 100644
--- a/test/CommonTestBase.t.sol
+++ b/test/CommonTestBase.t.sol
@@ -43,9 +43,7 @@ contract CommonTestBase is PRBTest {
configStore = new ConfigStore();
stakedToken = new MockERC1155("");
- contestFactory = new ContestFactory(address(stakedToken), stakeableTokenIds);
toggleGovernanceFactory = new ToggleGovernanceFactory();
- configStore.changeImplementationAddress(ConfigStoreInterfaces.CONTEST_FACTORY, address(contestFactory));
configStore.changeImplementationAddress(ConfigStoreInterfaces.BENEFICIARY, address(beneficiary));
configStore.changeImplementationAddress(ConfigStoreInterfaces.TEAM, address(beneficiary));
configStore.changeImplementationAddress(ConfigStoreInterfaces.TEAM_PERCENT, address(25));
@@ -53,10 +51,13 @@ contract CommonTestBase is PRBTest {
ConfigStoreInterfaces.TOGGLE_GOVERNOR_FACTORY, address(toggleGovernanceFactory)
);
- contestStaker = ContestStaker(contestFactory);
spire = new SpireHarness(configStore);
+ contestFactory = new ContestFactory(address(stakedToken), stakeableTokenIds, spire);
+ contestStaker = ContestStaker(contestFactory);
+ configStore.changeImplementationAddress(ConfigStoreInterfaces.CONTEST_FACTORY, address(contestFactory));
+
+
spire.init();
- contestFactory.setSpire(address(spire));
vm.stopPrank();
}
}By applying these changes, it's also no longer necessary for ContestFactory to inherit from Ownable since the only use of the onlyOwner modifier has been removed.
Resolution:
The recommendation has been implemented.
In addition to simply initializing the _spire variable in the ContestFactory as in the recommendation, the check from the setSpire function has been implemented there as well.
Also the SetSpireAddress event is emitted.
Description:
The ToggleGovernance.hasEnoughOpenGovernToggles function (Link) is only called from within Spire._shouldSkipGenesisText (Link).
The call to ToggleGovernance.hasEnoughOpenGovernToggles can only be reached when flow == Flow.Chapter and nextChapterId[genesisTextId] < GlobalConstants.MAX_CHAPTER_COUNT, i.e. 0 <= nextChaperId[genesisTextId] < 100.
This means the requiredOpenToggles parameter for the ToggleGovernance.hasEnoughOpenGovernToggles function is in the range (1,100].
Since we know that governanceToken.totalSupply(governanceTokenId) = 100 (Genesis Text tokens are minted before the chapter flow can start advancing), the requiredOpenToggles > governanceToken.totalSupply(governanceTokenId) condition never evaluates to true.
Recommendation:
Based on the above considerations we can simplify the ToggleGovernance.hasEnoughOpenGovernToggles function like so:
diff --git a/src/ToggleGovernance.sol b/src/ToggleGovernance.sol
index afdba10..530413b 100644
--- a/src/ToggleGovernance.sol
+++ b/src/ToggleGovernance.sol
@@ -47,10 +47,6 @@ contract ToggleGovernance is ERC1155Holder, IToggleGovernance, ReentrancyGuard,
// staked = "closed" toggles
// unstaked = "open" toggles
// inital = all of "open" toggles
- // Threshold for staked amount is minimum of total supply and required stake.
- uint256 threshold = requiredOpenToggles > governanceToken.totalSupply(governanceTokenId)
- ? governanceToken.totalSupply(governanceTokenId)
- : requiredOpenToggles;
- return GlobalConstants.MAX_CHAPTER_COUNT - stakedAmount >= threshold;
+ return GlobalConstants.MAX_CHAPTER_COUNT - stakedAmount >= requiredOpenToggles;
}
}Resolution:
The recommendation has been implemented.
Description:
Both _teamPercent and _teamWallet are only read once in this function.
Therefore it does not save Gas to write the result of _getTeamPercent() and _getTeamWallet() to memory variables.
Use _getTeamPercent() and _getTeamWallet() directly. (Link).
Resolution:
The recommendation has been implemented.
Description:
In the MintContestReward event, GlobalConstants.CONTEST_REWARD_AMOUNT is emitted in the amount parameter.
This is not a meaningful value as it is a constant and it's not the value that is actually minted to the winner. It is recommended to emit GlobalConstants.CONTEST_REWARD_AMOUNT - _teamCount instead. (Link).
Resolution:
The recommendation has been implemented.
Description:
The Echoes._setInitialEchoContestWinners function (Link) just passes it's arguments on to the Echoes._setEchoContestWinnner function without providing any additional abstraction.
Apparently Echoes._setInitialEchoContestWinners is a remnant of a previous iteration of the protocol and it's no longer needed.
The function is only called in Spire._setLatestEchoContestWinners.
Instead, Spire._setLatestEchoContestWinners should call Echoes._setEchoContestWinner directly.
diff --git a/src/Echoes.sol b/src/Echoes.sol
index d14833d..39be41e 100644
--- a/src/Echoes.sol
+++ b/src/Echoes.sol
@@ -103,21 +103,6 @@ contract Echoes is HasConfigStore, ERC1155Supply {
}
}
- // Reverts if there isn't exactly one winning ID per initial echo contest.
- // Will skip any contest ID's that have already set a winner or can't select a winner yet.
- // Will revert if winning ID is not an actual entry ID for any of the contests that we can set a winner for.
- function _setInitialEchoContestWinners(
- uint256 genesisTextId,
- uint256 chapterId,
- uint256 echoId,
- uint256 winningId,
- uint256[] memory losingEntryIds
- )
- internal
- {
- _setEchoContestWinner(genesisTextId, chapterId, echoId, winningId, losingEntryIds);
- }
-
// Approve entryIds for echo ID.
function _approveEchoContestEntries(
uint256 genesisTextId,
diff --git a/src/Spire.sol b/src/Spire.sol
index cba361f..f743505 100644
--- a/src/Spire.sol
+++ b/src/Spire.sol
@@ -518,7 +518,7 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
)
internal
{
- _setInitialEchoContestWinners(
+ _setEchoContestWinner(
genesisTextId, _latestEchoChapterId(genesisTextId), echoId, winningId, losingEntryIds
);
}Resolution:
The recommendation has been implemented.
In the call to _setEchoContestWinner, an additional toBeneficiary parameter is passed which is in line with the recommendation for Finding 1.
Resolution:
The recommendation has been implemented.
Resolution:
The recommendation has been implemented.
Resolution:
The recommendation has been implemented.
Resolution:
The recommendation has been implemented.
Resolution:
The recommendation has been implemented.
Description:
The name of this function is a leftover from a previous iteration of the protocol.
setEchoContestWinner more accurately describes the behavior of the function.
Note that there are downstream functions which have the same naming mistake.
Rename those as well even though they're not public.
Resolution:
The recommendation has been implemented.
Description:
It is only necessary to perform the chapterEchoes[genesisTextId][chapterId][i] == address(0) check once (for any of the initial echo contests).
Either all initial echo contests exist or they don't.
diff --git a/src/Echoes.sol b/src/Echoes.sol
index d14833d..9988075 100644
--- a/src/Echoes.sol
+++ b/src/Echoes.sol
@@ -201,11 +201,11 @@ contract Echoes is HasConfigStore, ERC1155Supply {
}
function _initialEchoContestsHaveWinners(uint256 genesisTextId, uint256 chapterId) internal view returns (bool) {
+ if (chapterEchoes[genesisTextId][chapterId][0] == address(0)) {
+ return false;
+ }
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
- if (
- chapterEchoes[genesisTextId][chapterId][i] == address(0)
- || !IContest(chapterEchoes[genesisTextId][chapterId][i]).hasWinner()
- ) return false;
+ if (!IContest(chapterEchoes[genesisTextId][chapterId][i]).hasWinner()) return false;
}
return true;
}Resolution:
The recommendation has been implemented.
Description:
Some execution paths in the Spire.setEchoContestWinners and Spire.setChapterContestWinner functions require that the flow IDs are read multiple times from storage.
Saving the flow IDs to memory instead will save Gas when averaged over multiple function calls.
diff --git a/src/Spire.sol b/src/Spire.sol
index cba361f..cf8e856 100644
--- a/src/Spire.sol
+++ b/src/Spire.sol
@@ -207,14 +207,16 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
onlyRole(GlobalConstants.MID_ADMIN_ROLE)
nonReentrant
{
+ uint256 previousFlow = previousEchoFlowId;
+ uint256 currentFlow = currentEchoFlowId;
// Can only set current or previous echo contest winners.
- if (genesisTextId != previousEchoFlowId && genesisTextId != currentEchoFlowId) {
+ if (genesisTextId != previousFlow && genesisTextId != currentFlow) {
revert InvalidGenesisTextIdSetEchoContestWinner();
}
// Must set previous echo contest winner before current one.
- if (genesisTextId == currentEchoFlowId && currentEchoFlowId != previousEchoFlowId) {
- if (!_latestEchoContestsHaveWinners(previousEchoFlowId)) revert MustSetPreviousEchoContestWinners();
+ if (genesisTextId == currentFlow && currentFlow != previousFlow) {
+ if (!_latestEchoContestsHaveWinners(previousFlow)) revert MustSetPreviousEchoContestWinners();
}
_setLatestEchoContestWinners(genesisTextId, winningId, echoId, losingEntryIds);
_advanceEchoFlowIfPossible();
@@ -230,14 +232,16 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
onlyRole(GlobalConstants.MID_ADMIN_ROLE)
nonReentrant
{
+ uint256 previousFlow = previousChapterFlowId;
+ uint256 currentFlow = currentChapterFlowId;
// Can only set current or previous chapter contest winners.
- if (genesisTextId != previousChapterFlowId && genesisTextId != currentChapterFlowId) {
+ if (genesisTextId != previousFlow && genesisTextId != currentFlow) {
revert InvalidGenesisTextIdSetChapterContestWinner();
}
// Must set previous chapter contest winner before current one.
- if (genesisTextId == currentChapterFlowId && currentChapterFlowId != previousChapterFlowId) {
- if (!_latestChapterContestHasWinner(previousChapterFlowId)) revert MustSetPreviousChapterContestWinner();
+ if (genesisTextId == currentFlow && currentFlow != previousFlow) {
+ if (!_latestChapterContestHasWinner(previousFlow)) revert MustSetPreviousChapterContestWinner();
}
_setLatestChapterContestWinner(genesisTextId, winningId, losingEntryIds);
_advanceChapterFlowIfPossible();Resolution:
The recommendation has been implemented.
36. Spire._advanceEchoFlowIfPossible and Spire._advanceChapterFlowIfPossible: Gas optimization suggestions
Description:
nextEchoChapterId[newCurrentFlowId] does not need to be saved to memory, it's only ever read once.
Use the newCurrentFlowId memory variable instead of the currentEchoFlowId storage variable when calling _createNextEchoContests.
Use the newCurrentFlowId memory variable instead of the currentChapterFlowId storage variable when calling _createNextChapterContest.
diff --git a/src/Spire.sol b/src/Spire.sol
index cba361f..c21d1db 100644
--- a/src/Spire.sol
+++ b/src/Spire.sol
@@ -292,8 +292,7 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
(bool canAdvance, uint256 newCurrentFlowId, uint256 newPreviousFlowId) =
_advanceFlowIfPossible(previousEchoFlowId, currentEchoFlowId, Flow.Echo);
if (canAdvance) {
- uint256 _nextEchoChapterId = nextEchoChapterId[newCurrentFlowId];
- if (_nextEchoChapterId > 0) {
+ if (nextEchoChapterId[newCurrentFlowId] > 0) {
uint256 latestEchoId = _latestEchoChapterId(newCurrentFlowId);
// The chapter contest should be created and must have set a winner before we can open its echoes.
if (!_hasCreatedChapterContest(newCurrentFlowId, latestEchoId)) {
@@ -309,7 +308,7 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
emit AdvancedEchoFlow(newPreviousFlowId, newCurrentFlowId);
// If next genesis text hasn't opened its echo contests yet then open them.
- _createNextEchoContests(currentEchoFlowId);
+ _createNextEchoContests(newCurrentFlowId);
}
return canAdvance;
}
@@ -355,7 +354,7 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
previousChapterFlowId = newPreviousFlowId;
emit AdvancedChapterFlow(newPreviousFlowId, newCurrentFlowId);
- _createNextChapterContest(currentChapterFlowId);
+ _createNextChapterContest(newCurrentFlowId);
}
return canAdvance;
}Resolution:
The recommendation has been implemented.
37. Spire.approveChapterContestEntries and Spire.approveEchoContestEntries: Gas optimization suggestions
Description:
Save the current flow ID to memory instead of reading it from storage twice.
diff --git a/src/Spire.sol b/src/Spire.sol
index cba361f..b4f024e 100644
--- a/src/Spire.sol
+++ b/src/Spire.sol
@@ -396,7 +396,8 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
onlyRole(GlobalConstants.MID_ADMIN_ROLE)
nonReentrant
{
- IContest(chapterContests[currentChapterFlowId][_latestChapterId(currentChapterFlowId)]).acceptEntries(entryIds);
+ uint256 currentChapterFlow = currentChapterFlowId;
+ IContest(chapterContests[currentChapterFlow][_latestChapterId(currentChapterFlow)]).acceptEntries(entryIds);
_advanceChapterFlowIfPossible();
}
@@ -416,7 +417,8 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
onlyRole(GlobalConstants.MID_ADMIN_ROLE)
nonReentrant
{
- _approveEchoContestEntries(currentEchoFlowId, _latestEchoChapterId(currentEchoFlowId), echoId, entryIds);
+ uint256 currentEchoFlow = currentEchoFlowId;
+ _approveEchoContestEntries(currentEchoFlow, _latestEchoChapterId(currentEchoFlow), echoId, entryIds);
_advanceEchoFlowIfPossible();
}Resolution:
The recommendation has been implemented.
Description:
The echo flow and the chapter flow should be independent as much as possible.
The initial echo contests for a chapter (I will refer to them from now on as a single "echo contest") can be opened whenver the chapter contest has set a winner.
And a new chapter contest for a genesis text can be opened when the echo contest for the previous chapter contest of the same genesis text has set a winner.
(There are more conditions for opening contests, but these are the one's of interest)
Both the echo flow and the chapter flow might not be able to advance and open a new contest even though it's possible to do so.
Impact:
This finding is only rated as an Improvement because this behavior is not clearly wrong, it's simply a time-inefficieny.
In other words it might take longer for new contests to open (both chapter contests and echo contests) than is necessary.
This cannot lead to the Spire getting stuck however as is the case with the two High severity findings.
A scenario in which the echo flow cannot advance is illustrated in the following test:
// add test to Spire.t.sol
function test_echoFlowDoesNotFindNextChapter() public {
// initialize genesis echoes
// close and select winner for all genesis echo contests except last one (5)
uint256 winningId = spireUtils.completeGenesisEchoes(0);
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(0, winningId, i, new uint256[](0));
}
winningId = spireUtils.completeGenesisEchoes(1);
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(1, winningId, i, new uint256[](0));
}
winningId = spireUtils.completeGenesisEchoes(2);
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(2, winningId, i, new uint256[](0));
}
winningId = spireUtils.completeGenesisEchoes(3);
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(3, winningId, i, new uint256[](0));
}
winningId = spireUtils.completeGenesisEchoes(4);
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(4, winningId, i, new uint256[](0));
}
winningId = spireUtils.completeGenesisEchoes(5);
console.log(Contest(spire.chapterEchoes(0,0,0)).hasWinner());
console.log(Contest(spire.chapterEchoes(1,0,0)).hasWinner());
console.log(Contest(spire.chapterEchoes(2,0,0)).hasWinner());
console.log(Contest(spire.chapterEchoes(3,0,0)).hasWinner());
console.log(Contest(spire.chapterEchoes(4,0,0)).hasWinner());
console.log(Contest(spire.chapterEchoes(5,0,0)).hasWinner());
// skip chapter contests for genesis texts 0 and 1
ToggleGovernance tg0 = ToggleGovernance(spire.genesisGovernors(0));
vm.startPrank(spire.owner());
spire.setApprovalForAll(address(tg0), true);
tg0.stake(GlobalConstants.MAX_CHAPTER_COUNT);
vm.stopPrank();
assertTrue(spire.shouldSkipGenesisText(0, Spire.Flow.Chapter));
ToggleGovernance tg1 = ToggleGovernance(spire.genesisGovernors(1));
vm.startPrank(spire.owner());
spire.setApprovalForAll(address(tg1), true);
tg1.stake(GlobalConstants.MAX_CHAPTER_COUNT);
vm.stopPrank();
assertTrue(spire.shouldSkipGenesisText(1, Spire.Flow.Chapter));
console.log(IERC1155(address(spire)).balanceOf(spire.owner(),0));
console.log(IERC1155(address(spire)).balanceOf(spire.owner(),1));
console.log(IERC1155(address(spire)).balanceOf(spire.owner(),2));
// initialize chapter flow at genesis text 2
spire.initializeChapterFlow();
console.log(spire.getChapterContests(0,0));
console.log(spire.getChapterContests(1,0));
console.log(spire.getChapterContests(2,0));
// now skip all genesis texts apart from 1
ToggleGovernance tg2 = ToggleGovernance(spire.genesisGovernors(2));
vm.startPrank(spire.owner());
spire.setApprovalForAll(address(tg2), true);
tg2.stake(GlobalConstants.MAX_CHAPTER_COUNT);
vm.stopPrank();
assertTrue(spire.shouldSkipGenesisText(2, Spire.Flow.Chapter));
ToggleGovernance tg3 = ToggleGovernance(spire.genesisGovernors(3));
vm.startPrank(spire.owner());
spire.setApprovalForAll(address(tg3), true);
tg3.stake(GlobalConstants.MAX_CHAPTER_COUNT);
vm.stopPrank();
assertTrue(spire.shouldSkipGenesisText(3, Spire.Flow.Chapter));
ToggleGovernance tg4 = ToggleGovernance(spire.genesisGovernors(4));
vm.startPrank(spire.owner());
spire.setApprovalForAll(address(tg4), true);
tg4.stake(GlobalConstants.MAX_CHAPTER_COUNT);
vm.stopPrank();
assertTrue(spire.shouldSkipGenesisText(4, Spire.Flow.Chapter));
ToggleGovernance tg5 = ToggleGovernance(spire.genesisGovernors(5));
vm.startPrank(spire.owner());
spire.setApprovalForAll(address(tg5), true);
tg5.stake(GlobalConstants.MAX_CHAPTER_COUNT);
vm.stopPrank();
assertTrue(spire.shouldSkipGenesisText(5, Spire.Flow.Chapter));
vm.startPrank(spire.owner());
tg1.unstake(GlobalConstants.MAX_CHAPTER_COUNT);
vm.stopPrank();
assertFalse(spire.shouldSkipGenesisText(1, Spire.Flow.Chapter));
// select winner for chapter contest at genesis text 2
// next chapter contest is created for genesis text 1
spireUtils.completeLatestChapterContest(2);
vm.startPrank(spire.owner());
spire.setChapterContestWinner(2, 0, new uint256[](0));
vm.stopPrank();
console.log(spire.getChapterContests(0,0));
console.log(spire.getChapterContests(1,0));
console.log(Contest(spire.getChapterContests(1,0)).hasWinner());
console.log(spire.getChapterContests(2,0));
console.log(Contest(spire.getChapterContests(2,0)).hasWinner());
console.log(spire.previousEchoFlowId());
console.log(spire.currentEchoFlowId());
// select winner for genesis echo contest at genesis text 5
for (uint256 i = 0; i < GlobalConstants.INITIAL_ECHO_COUNT; i++) {
vm.prank(spire.owner());
spire.setEchoContestWinners(5, winningId, i, new uint256[](0));
}
// echo flow cannot advance even though there is a suitable contest to advance to!!
console.log(spire.previousEchoFlowId());
console.log(spire.currentEchoFlowId());
}Recommendation:
This finding has been discussed with the client at length.
It has been determined that in the case of the chapter flow having to wait for the echo flow, time efficiency is not the main concern.
The main concern is predictability with regards to which chapter contest opens next.
On the other hand, the client decided to adjust the logic of the echo flow.
Consider this scenario:
Currently, the echo flow would have to wait for the chapter contest at genesis text 1 to select a winner.
The intended new behavior is this:
Instead of waiting for the chapter contest at genesis text 1 to select a winner, the echo flow moves on to the next chapter contest that has selected a winner which is at genesis text 2.
The new behavior can be implemented by tightening the rules for not skipping genesis texts in the Spire._shouldSkipGenesisText function (Link).
diff --git a/src/Spire.sol b/src/Spire.sol
index cba361f..1e77d2b 100644
--- a/src/Spire.sol
+++ b/src/Spire.sol
@@ -426,7 +426,13 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
}
if (flow == Flow.Echo) {
- if (nextEchoChapterId[genesisTextId] == nextChapterId[genesisTextId]) {
+ uint256 _nextEchoChapterId = nextEchoChapterId[genesisTextId];
+
+ if (_nextEchoChapterId == 0) {
+ return false;
+ } else if (_hasCreatedChapterContest(genesisTextId, _nextEchoChapterId - 1) &&
+ IContest(chapterContests[genesisTextId][_latestChapterId(genesisTextId)]).hasWinner())
+ {
return false;
} else {
return true;The first echo contest for a genesis text can always open (_nextEchoChapterId == 0), otherwise we require that the chapter contest is created and has set a winner.
In addition it's possible to remove a case differentiation in the Spire._advanceFlowIfPossible function (Link) that has been introduced to handle a situation in which two genesis texts are open.
By applying the above changes, this is handled correctly by default.
diff --git a/src/Spire.sol b/src/Spire.sol
index cba361f..d4be0ee 100644
--- a/src/Spire.sol
+++ b/src/Spire.sol
@@ -268,21 +268,7 @@ contract Spire is ISpire, GenesisText, ERC1155Holder, AccessControl, Ownable, Re
if (latestContestsHaveClosed && previousContestHasWinner) {
canAdvance = true;
- if (flow == Flow.Chapter) {
- (newCurrentFlowId, newPreviousFlowId) = _incrementFlowCounter(currFlowId, prevFlowId, flow);
- } else {
- uint256 latestEchoId = _latestEchoChapterId(currFlowId);
- if (
- _hasCreatedChapterContest(currFlowId, latestEchoId)
- && IContest(chapterContests[currFlowId][latestEchoId]).hasWinner()
- ) {
- // A special case where chapter and echoe are at the same position
- newCurrentFlowId = currFlowId;
- newPreviousFlowId = currFlowId;
- } else {
- (newCurrentFlowId, newPreviousFlowId) = _incrementFlowCounter(currFlowId, prevFlowId, flow);
- }
- }
+ (newCurrentFlowId, newPreviousFlowId) = _incrementFlowCounter(currFlowId, prevFlowId, flow);
} else {
canAdvance = false;
}Resolution:
The fix has been made exactly as recommended, both diffs have been applied.
Furthermore there are no conflicting changes.