Citadel A-1

Security Audit

December 2nd, 2022

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Citadel's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from September 5, 2022 to November 12, 2022.

The purpose of this audit is to review the source code of certain Citadel Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
Critical 1 - - 1
High 9 - - 9
Medium 17 - - 17
Low 18 2 1 15
Code Quality 21 - - 21
Informational 2 - - -
Gas Optimization 6 - - 6

Citadel was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts as part of the main audit:

Contract SHA256
contracts/Migrations.sol

834be0f6545fff11a6f1c9cb7846f8faf4ccf9a27c004413d73459ed7c4e85ed

contracts/bridging/BaseChildTunnel.sol

a2205ed9c7e42de550317c0fd5b6c4ae2f64aa85c2b9552185de124463bc0807

contracts/bridging/BaseRootTunnel.sol

b4b95b5dd84d46fe637ea412e9c055f3f898e1cb0c0654a5625957dc7ee25a42

contracts/bridging/BaseRootTunnelMock.sol

c5161e19f99d6331bd7814a73b66891e84c642ae34153cf0c8445ccdc7b3145e

contracts/bridging/Dock.sol

6f33bdd8a428e62a86707e37b49e23edcfd0565651b36a481c4e62c227a12ff9

contracts/bridging/GovernanceChildTunnel.sol

3e736ef5140b0c626fc58bcd07026d5ac871b8b4fe1c9a250fc3592699311980

contracts/bridging/GovernanceRootTunnel.sol

b8f65180782a1e130019571799a3108cffa2057b0ca4f6a04813f33a8e4fade7

contracts/bridging/GovernanceRootTunnelMock.sol

26904b0453d682491b09436b786fe1f9f909f2c1373f8be6b27ca6885cfdfbc8

contracts/bridging/StandardChildTunnel.sol

caf8ad3fafa71d31a1a9a43ce3a009752d9c3df14b66ed34e659027d716b317c

contracts/bridging/StandardRootTunnel.sol

a288bdabe60b1c49a0be7ec754f12946805a89e373fe142694eb8cdc9ab18a5d

contracts/bridging/StandardRootTunnelMock.sol

e4b851ed961b68ce7a8c133d15377750923dc88bd3111a66e880fbd468b0aa48

contracts/construction/ConstructionBay.sol

cd443a4a741ed1f80344fcf59689f12d932a81954aea7872a69655f4a36ab721

contracts/finance/CitadelPaymentSplitter.sol

71fcb5af1e68909f0d26917e68cef5b1aafd8bc68142c5e73cacba4597a682ab

contracts/fx/FxChildMock.sol

b87e44ba2770ca369de8098885b30d78260d55ac573eeacf04abca48855d33d6

contracts/fx/FxRootMock.sol

4c4abf29efc9e47defbc57e27e55c76ff3684d0e412c7c2632c988d0f7c92109

contracts/fx/lib/ExitPayloadReader.sol

624288703e467b956f7c04e0aaf6b97d538eca8d888f67f7a7ffe66468c0eb91

contracts/fx/lib/Merkle.sol

ecc7cd00047388c0f2612b8086f457162b2d99d93c6141447e4682008fd6324e

contracts/fx/lib/MerklePatriciaProof.sol

5086800b71b49202d05b54984e4a0970dbbff8aef6e62e77b9a63755a96e56a1

contracts/fx/lib/RLPReader.sol

b700843f24e26dea92ba70c9748a305355dcf5a5f08b9adb65b0f57fe4f0c653

contracts/fx/tunnel/FxBaseChildTunnel.sol

1e80c9d91361c1fba458c4f8302c59ea4ab108ca49fb00b2f8ebad13e008af2f

contracts/fx/tunnel/FxBaseRootTunnel.sol

13e41b33a1c03ffcab1ab91d2e0598d636e4436838fdc8ff5d5d00b68e8a828a

contracts/game/Belts.sol

e51994cb8fb51b929063f8f3c09152e31cafcd3a315809bc16676da3cf2f2139

contracts/game/BeltsLib.sol

89c10b4e41881a8ef762fbdcbd999c694a089cd0862fbee1f0ce58fa24eb7a75

contracts/game/Game.sol

52acb1986a754cf0e5084582c313654b12fe78de9783045816125ddc294cbf00

contracts/game/Outposts.sol

809539b22ad1c06bfef8978690b59f0a9108a1609864b6be28e2e2a667544d82

contracts/game/OutpostsLib.sol

2674953b6930a6dfe21656fbf44795be231d63db6beb5e4e2c39c8080d33112f

contracts/game/Rewards.sol

61264dd212f59c106d27e62cbce16fd5395b70a566cf798dab2eff6aae25a594

contracts/game/RewardsLib.sol

e3c3860ded7e3ff2e287cdabd3422754efe6a8375c2b8b83d80cf2d4cf3c8434

contracts/game/Staking.sol

c5aa6f08306e2a668a64119b959874afe12a2384617717fa8ef2ec41bd0b6b9b

contracts/game/TokenTraits.sol

3667f036e4dc978ddec9cb62690d40362ed065b1270e9568204464e931005c30

contracts/game/Travel.sol

3d844cd042efeaca67a884a0972b390af72733da1aefcfc5994c24f0b38289d2

contracts/game/TravelLib.sol

f37677edd619126d73e555a0794168a74b7dc6bdae5a24ebe75776b77083a9e7

contracts/game/Upgrades.sol

64875fdbc974dde047868819c7d63eb1c8daafe9c556df8e741759f52f5269fd

contracts/game/UpgradesCalculator.sol

fb961583ac937e2f050d0e94911c0ec61b3e1244b6e4a4a447d03052625871d2

contracts/game/UpgradesCalculatorLib.sol

28a0773cca46da481a14263eedc58fc697150069ce6894e4d50125f8afc56df0

contracts/game/UpgradesLib.sol

efa86ca6aca76c7749e834cad04849959aade5776c376391a3c05fa0a54ff95b

contracts/governance/CitadelGovernor.sol

292e648abe7839300d60d7d88d4a4bd85ee7c2d083b3c215ffe4b6283b8463ca

contracts/governance/CitadelTimelock.sol

37a05a9092c840a40161c162043fa03fb3f3b3b3faf7ebfc5916d38d30f2dbca

contracts/governance/GovernanceTokenMock.sol

5864e47c9e698de36d3918aadb676c2840af0b440e94c0863f1f76231e031fbe

contracts/interfaces/IBeltsLib.sol

fb550a338688809ded088b8cedf7808d1ef0b7aa646f06a814142ffbb36ed1e0

contracts/interfaces/IConstructionBay.sol

5d1544037f163c1d3a999ad91bcdbf9f489487d313e1e18a5bf28fda93c957a9

contracts/interfaces/IERC20Burnable.sol

d567b4af0c9d1c78a2c2c5e29ea68e7e61377dc6b375c50c18c4caea54f4d39d

contracts/interfaces/IGameEvents.sol

82335157c0e2704ac500cea28dfc1ff4cd473c5eeb605803b7a67f9e07fe322d

contracts/interfaces/IMetadataLib.sol

1d7164f199c7edf05c1852cd32b77c1b067ed0e366f7780cf3024321c8f38882

contracts/interfaces/IOre.sol

664dbaa6e9441fd637d95bbe34325ad1a7057681f684ca49444b08e8bc9ae84e

contracts/interfaces/IOutpostsLib.sol

45fe77a3c67100a5808ec072a5725ca5f33056563288faaf056d00c81b940687

contracts/interfaces/IPilotBadge.sol

a88474279a02fd6156abe00bbe4e5926f2d284a89cf42b59ce427104a00d9465

contracts/interfaces/IProxyRegistry.sol

dad4566641fdc54baf6ca0df4d7c3ec09da21e45170446ca3e65be7bfe847167

contracts/interfaces/IRandomOracle.sol

c71a32f78f9bc4b974671a9375cd73824c641d7c620a15ed50b8e2ef6128e60c

contracts/interfaces/IRewardsLib.sol

10444a1209e641f12a397b18d9d97c484d9653c3145a69a9769112ae7f906968

contracts/interfaces/IShipTraits.sol

591a13d39dd9ba6f3b1e592dbcbcf8132837581e97c7ff1494e5cc849e6641f8

contracts/interfaces/IShips.sol

d2494fb0ed00edcc7c469c38d8b3478db3afd9449768f5d185272477bf881da8

contracts/interfaces/IStructs.sol

c9314212705fb1a31c58e9e0b471a4f576ccab95791862c77da76266b201f820

contracts/interfaces/ITravelLib.sol

63c1b6bb1895f8bc52aeabc1c9e020d7afffa30fa50d3bdc1ed88809052b034f

contracts/interfaces/ITunnel.sol

dad64f71c129a395b55cd9b44869836e0fb116da7ce3849e5948b4b319892e7b

contracts/interfaces/IUpgradesCalculatorLib.sol

822e0aa75e8ba5693bab3f44b5ecd3f43245cf38e37c65393626604b34809914

contracts/interfaces/IUpgradesLib.sol

6f70ace6eb4a95d5f3417e0f15aeecf2036f42cf87cc1862ccaeb2babf58f477

contracts/oracles/RandomOracle.sol

1cfc559ebea2e8254e9877a008f72381ebf1f4fc7fc7b221266e9dfe18b0567f

contracts/oracles/RandomOracleConsumer.sol

30087892c16c59fc4adc7cea158bff3ae042565c2275d5911b00f9474129b806

contracts/oracles/RandomOracleConsumerMock.sol

e37c18280a7b73001be943f9bf74fb38102e42f4d001f085faa6635d61a06410

contracts/oracles/RandomOracleMock.sol

0b6ec9cc92e461f78dfce5d145de6031db4a76a9c29cd1d7e7aa6c8e1ecb7ac7

contracts/proxy/CitadelProxyAdmin.sol

975f96c448ea3e14d3a9c86ce4f0fbf663a76ad73cfb822db75a71fe706c5563

contracts/proxy/TransparentUpgradeableProxyMock.sol

2ec505a5b63dd2c97999e7ff91856458b6a074819680296b84cdda9a4122e5f7

contracts/proxy/UpgradeableMockV1.sol

180c91624339786c236845bc6bf9fb4850ee6ead13242a49211ebff8c6786001

contracts/proxy/UpgradeableMockV2.sol

36c303f01782bcf18316a0d9a752084f9f347b8d872c69c4e0f541421dd0ccb8

contracts/token/BaseShipTraits.sol

15ab96fd68187383147989082c77eb00d3618450be6a3fa8f9dd003199b671d5

contracts/token/BaseShips.sol

3266d495a898f5113c6b31d6b50278ac26145301625961f483d2558dcad93c1e

contracts/token/DutchAuctionShips.sol

29c9c211c9416029953b49f6106df4c9c4e607fb5ec01d98420003b41aa94a3a

contracts/token/ERC20Locked.sol

695c6d44f74ce3801d6f5b295cc7e8cae7b4d6ca196cd193190d716db3ee2d9c

contracts/token/ERC20LockedVotes.sol

6b7554d5091eaba27d713c30147e372ca9c1fc31a773c374497d22abfcdc0efc

contracts/token/ERC721C.sol

9837502605dab69830bccc708c6a7be52bfbdeece77634d053a7f7072e7c9eb6

contracts/token/FixedPriceShips.sol

bc2b64f00f723ee97983ae1ed70015fff032242a8f10fade0cdf4cdc2b2d795e

contracts/token/FixedShipTraits.sol

357878c29a3be4b0e210c1a398d1bb8d309a89545de9ba93e56b54b51ffd4992

contracts/token/GenesisShipTraits.sol

9db8159a7a0ad011f42617233c684451a58703dde411c59ea682c735e505f8ef

contracts/token/MetadataLib.sol

34f6e30b47ed22bb28a986f97ed217e2ce29b53b35caef16d04ebf81c6b749ef

contracts/token/Ore.sol

d49ccecfa214ecc0ac62439287b21e74bd96d5d2786ae95438618b0b3de4b414

contracts/token/PilotBadge.sol

8bbf66d5b4794012fb0bbbb6d0f6cc1556a7410136668686b2017f2e60200973

contracts/token/ProxyRegistryMock.sol

c06c25851d4deb2ea3d5463a2c161c114a328e1678a8f7b9066f779b2a03cd14

contracts/token/ShipsMock.sol

71f4ef7c7c100ca5a4329d6945e6933cb0015fcce0946e1266abe913aa6f27b0

contracts/util/Bytes32Strings.sol

9c49d3ee872f8e27866d4086124ca38989d2f41bcb9286af8aa77b0072b27d40

contracts/util/TokenContracts.sol

a52603f725839259b0503864ffb8531bfe116c031dda556830e7860742350db7

We audited the following contracts and circuits as part of zero-knowledge implementation of Construction Bay:

Contract SHA256
contracts/construction/ConstructionBay.sol

4096fb22fba84ff0d905df3612dd3f3e0c9abf2b2457ef1db9f13e4bf5047894

contracts/construction/RedBlackTreeLib.sol

0c4cb761a86912e5da5963e0207f93b6854e46542b69b6f86c9a0d4715a4e3f7

contracts/interfaces/IConstructionBay.sol

8b5c03f64cdbad0ce999e17ffbc14003307b3518d05230165a6136e314f9cd86

contracts/verifiers/CommitVerifier.sol

e566e282f17cc2354dd5b82a1debb709080ab30f4b26bf1ca16c65cd18f13028

contracts/verifiers/RevealVerifier.sol

b2f1b90a47d58d3de8106e4d865fea15eae94c4d316cd069e44f9fc6b6573ad3

circuits/bid_ticket_hasher.circom

f746ab1bfe47af14f05ea3c0743a871ceec55acf4a5d493fa34ec756998156f2

circuits/commit.circom

61fd7ca75fd17feb3a98f9bc9d6ae2d8a6501d037b145eca3881474b0fb9c8e8

circuits/reveal.circom

75f1ee8f7e6a6452aefcce3352fe9b27d83180fd10fb6c737096d92d2ab2467e

We also reviewed following deployment scripts:

Contract SHA256
migrations/1_initial_migration.js

77b1cd797049373196a379f3cc406f94f81b72f7d754e8a3974c6bb7fee4d943

migrations/2_deploy_fx.js

997a832c4b2602a9ae4b1633d3a7965799f3d053dce6ddba48dd5a621ac77850

migrations/3_deploy_contracts.js

8567eb5d9df28e1778d87e8427884f5333d384c1094ab98dcf5785806506526e

migrations/4_enable_communication.js

dabc9930380a33f893477d990de6d03f7d43d2c45ad32cb351ca56f1ab66bd15

migrations/5_relinquish_to_governance.js

f2cbb2e88f25c5d861121e7a09ec2add781511d715165d27cc889832d89dd849

citadel-config.js

ef391bdaf0e552f842d7ec56727d909eb8d9f32a337190177ea8c582d7f8e3bc

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

C-1

Incorrect implementation of claimDepositorRewards may lead to insolvency of ConstructionBay

Topic
Insolvency
Status
Impact
High
Likelihood
High

In the ConstructionBay contract, the claimDepositorRewards function enables depositors to retrieve their share of total rewards.

Implementation is meant to enable multiple claims from the same depositor as rewards accrue in totalDepositorRewardsByAuction[auctionId] over time.

uint256 depositAmount = _depositByAuctionByAddress[auctionId](Msg.sender];

uint256 allTimeRewards = (totalDepositorRewardsByAuction[auctionId] *
    depositAmount) / totalOnTimeDepositsByAuction[auctionId];

uint256 owedRewards = allTimeRewards -
  _claimedRewardsByAuctionByAddress[auctionId](Msg.sender];

Specification defines that rewards can be claimed after the Punishing state has begun.

At any time after the punishing state has begun, depositors who revealed on time are eligible to claim their portion of the tax revenue for the auction.

However, with the current implementation, it is possible for depositors who have already revealed their bid to claim deposit rewards while the auction is still in state Revealing as there is no corresponding check to prevent that.

In this context, formula for calculating allTimeRewards is incorrect as both totalDepositorRewardsByAuction[auctionId] and totalOnTimeDepositsByAuction[auctionId] can change between subsequent calls leading to different, non-monotonically increasing, outputs.

This issue may lead to a loss of funds in the following scenario:

  1. All, except large whale depositor with 0 bid, reveal their bid

  2. All, except large whale depositor with 0 bid, claim their reward

  3. The sum of all rewards claimed at this moment would be equal totalDepositorRewardsByAuction[auctionId]

  4. Large whale depositor reveals their 0 bid increasing totalOnTimeDepositsByAuction[auctionId] significantly

  5. Large whale depositor claims depositor rewards and receives ~ totalDepositorRewardsByAuction[auctionId] since depositAmount of the whale depositor represents most of the totalOnTimeDepositsByAuction[auctionId] amount.

  6. As a result, the system distributes nearly 2x rewards at the expense of the DAO's balance share and other potential depositors who haven’t closed their position.

Remediations to Consider

  • Adding a guard to prevent reward claims unless the auction is in a Punishing state.
H-1

Advanced users can influence which ship traits they receive from auctions

Topic
Randomness
Status
Impact
High
Likelihood
High

From the yellow paper’s Ship Reveal section:

For ships introduced through the construction bay, we modulate the random seed to select each ship from a predefined list. Once a ship is selected, it is deleted from the list, so that no ship is selected more than once. For more details on the construction bay, see the Construction bay section.

More advanced users who have won a ship auction can influence what ship they receive by (1) pre-computing their received ship’s availableShip index and (2) only allowing a transaction to succeed if they receive a desired ship (by using services such as Flashbots Bundles). By waiting for other users to claim their ships, ship winners can change which ship they receive. This is due to how each claim changes which ship a reveal returns. This jeopardizes the fairness of auctions.

See the code in FixedShipTraits.sol lines 95-124:

/// @dev Reveal minted tokens' traits in separate transaction
function _revealTokens(uint256[] memory tokenIds, uint256[] memory seeds)
  internal
  override
  returns (Ship[] memory)
{
  require(
    availableShips.length >= tokenIds.length,
    "Not enough ships are available right now"
  );

  uint256 tokenIdsLength = tokenIds.length;

  Ship[] memory result = new Ship[](tokenIds.length);
  for (uint256 i = 0; i < tokenIdsLength; ) {
    /* calculated index will change as ships are claimed */
    uint256 index = seeds[i] % availableShips.length;
    Ship memory ship = availableShips[index];

    /* which ship an index points too also changes */
    availableShips[index] = availableShips[availableShips.length - 1];
    availableShips.pop();

    result[i] = ship;

    unchecked {
      i++;
    }
  }

  return result;
}

This becomes more problematic when a user controls more than one ship in the auction. Every ship they own results in greater influence over the resulting moduli and indexing operations. This attack could become economically worthwhile if there is a high demand for a ship in the auction.

Remediations to Consider

Consider implementing a different design which does not let users influence which ship they receive, such as:

  • Revealing all of the auction’s ship traits at the same time based off of a single random number. This should be done after users have claimed their shipTokenIDs as users have control over which shipTokenID* they receive based off of the order they announce their wins.
  • The code in ConstructionBay.sol on lines 561-572 assigns the shipTokenIDs in ascending linear order based on claiming order. This code is also influenceable by advanced users waiting for other winners to claim their shipTokenIDs.
H-2

Funds can get locked in ConstructionBay contract

Topic
Asset Mobility
Status
Impact
High
Likelihood
Medium

Let’s consider the scenario where no one of the auction participants revealed their bid on time (by not calling the revealBidOnTime function in time).

If so, the only action one can take is to call the punish function which does one of the following:

  1. If the bid is a winning bid, no-show tax (default: 50%) and flat tax (default: 5%) are deducted from the bidder’s balance and added to the totalDepositorRewardsByAuction mapping:

    totalDepositorRewardsByAuction[auctionId] += totalTaxAmount; // 55% of deposit
    

    and the remaining 45% are returned to the bidder.

    _sendToken(auctionId, ticket.bidder, remainingBalance);
    
  2. If the bid is not a winning bid, only flat-tax (default: 5%) is deducted from the bidder’s balance

    totalDepositorRewardsByAuction[auctionId] += taxAmount;  // 5% of deposit
    

    and the remaining 95% are returned to the bidder.

In either of the above cases, tax amount is added to the totalDepositorRewardsByAuction mapping. This total tax amount would then be used in the claimDepositorRewards function to give bidders their proportion of the rewards.

Now the problem is that only bidders who revealed on time are eligible to claim their portion of the rewards by calling claimDepositorRewards. Hence, in our scenario above, no bidder can claim his proportion of the rewards. As the contract provides no other way of transferring rewards out, those rewards are locked in the contract.

We see this issue as Medium likelihood as there can be multiple cases where no bidders reveal on-time:

  • Not many bidders on the auction and the few who participated didn’t reveal on time
  • finalityDelay is set to a low value and so bidders don’t have a long enough time to reveal
  • frontend outage during the reveal period and so there is no easy way for bidders to reveal

Remediations to Consider

  • Adding a deadline until when rewards can be claimed. If deadline is passed, rewards can be sent to DAO address.
H-3

Unsigned tickets will lock deposits

Topic
Asset Mobility
Status
Impact
High
Likelihood
High
Client opted to implement a zero-knowledge version of Construction Bay.

In the ConstructionBay.sol, there is no alternative to access funds if one of the signatures is invalid or missing. If the system does not sign a bid, the deposit cannot be revealed or punished, causing the funds to get locked in the contract.

There might be various reasons for an unsigned bid, the most frequent one being the off-chain system not signing bids if the requester does not have sufficient balance to cover the total and tax. From the yellowpaper:

The system must only sign a bid if the requester has sufficient balance to cover the total and tax.

The same issue is valid with any other case that blocks ticket signing, such as the unavailability of the off-chain system.

Remediations to Consider

  • A governance-only function to refund affected depositors.
H-4

Miners can pre-compute mining claim and can act accordingly if it results in ship destruction

Topic
Randomness
Status
Impact
High
Likelihood
High

The mining operation consists of the following 3 transactions: 1) beginMining 2) completeMining 3) revealMiningClaim. In the completeMining call, a random number is requested from the oracle that is later be used in the revealMiningClaim to calculate the crime outcome. If the crime outcome is 100%, then the ship will be burned.

Once the random number is returned and before calling revealMiningClaim, advanced users can pre-compute the crime outcome, and if the ship is destined to be destroyed, they are incentivised to not call revealMiningClaim.

Instead, they have the following 2 options:

1. Unstake and sell the ship

Miners can complete mining and returning to the Citadel by calling completeMining with arguments isReturning=true and outpostId=0. In this case - as they are in the Citadel - miners could unstake their ship instead of letting the ship to be destroyed by calling the revealMiningClaim.

After unstaking the “to be destroyed” ship, a malicious user could sell the ship on some marketplace such as OpenSea. At this time, the buyer wouldn’t know the internal game state of the ship. Once the new buyer enters the game by staking the ship, the ship is in a “pending claim” state, meaning that the player needs to claim the rewards first before completing another mining operation. If buyer now calls revealMiningClaim, the ship will be destroyed.

Remediations to Consider

  • In the unstake function, require the ship to not be in Claiming state.

2. Retain voting privilege by never revealing the mining claim

Miners are incentivized to not finish the reveal and to just keep their ship in the pending claim state in order to not have their pilot badge burned. This way, they can retain their voting privileges associated with the pilot badge.

In consequence, this will prevent marauders from receiving the ore which corresponds to the 100% theft category as miners will not reveal these ship-destroying mining claims.

Remediations to Consider

  • Incentivizing the miners who have had their ships destroyed to finish the reveal to properly reward the marauders, or
  • Instrumenting another way for these ships to be destroyed which isn’t driven by the negatively impacted party.
H-5

Misconfigured belts will lock all ships mining on them

Topic
Asset Mobility
Status
Impact
High
Likelihood
Medium

When a misconfigured (probabilities of crimeOutcomes or oreBreakdown not adding up to 100% or _oreBreakdown.length == 0 etc) belt gets added using addBelt() in Game.sol, provenance of the belt gets saved in _provenanceByBeltId. Since the publishBelt() in RewardsLib.sol checks the integrity of the published belt by comparing it with provenance, there is no way to publish or remove this belt, causing the belt to exist in an Unpublished state forever.

Unpublished belts are still available for ships to travel and start mining but ships can not completeMining() on them. This will result in a location that locks all ships mining on it without recourse.

Remediations to Consider

  • Blocking traveling to non-published belts
  • Allowing miners to stop mining non-published belts with no rewards
  • Preventing misconfigurations in the first call to addBelt()
H-6

RedBlackTreeLib.sol atIndex() can return wrong values and can revert

Topic
Input Ranges
Status
Impact
High
Likelihood
High

The current implementation of RedBlackTree.sol’s atIndex() function on line 165 is both wrong and can revert. This will cause ConstructionBay.sol’s submitWinningBid() function to either (1) return the wrong winning bid amount, which goes against the spec, or (2) to itself revert and prevent anyone from claiming ships.

The condition starting on line 197 is using the wrong cursorSelfCount when calculating the amount to decrease the current count of the considered nodes. This will both return wrong indexes and can underflow when indexing lower indexes.

// In RedBlackTree.sol's atIndex() 
} else {
  cursor = c.left;
  c = self.nodes[cursor]
  // this can underflow and lead to the wrong index being returned
  smaller -= (cursorSelfCount + nodeCount(self, c.right)); 
}

The fix would be:

} else {
  cursor = c.left;
  c = self.nodes[cursor];
  cursorSelfCount = c.selfCount; // add this to line 198 of the tree impl
  smaller -= (cursorSelfCount + nodeCount(self, c.right));
}

Remediations to consider:

  • Adding the indicated line above.
H-7

Reveals will revert if user submits same bid as someone else in same auction

Topic
Input Ranges
Status
Impact
High
Likelihood
High

ConstructionBay.sol’s used RedBlackTree implementation does not allow for re-insertion of keys and reverts if one tries. See insert() on line 120 of RedBlackTreeLib.sol:

function insert(
    Tree storage self,
    uint256 key,
    uint256 count
  ) internal {
    require(key != EMPTY);
    require(!exists(self, key)); 
        ...

This becomes problematic as the current reveal() function on line 247 of ConstructionBay.sol does not check to see if the key is duplicated before attempting insertion.

function reveal(...) { 
    ...
    if (bid > 0 && quantity > 0) {
      _state[auctionId].totalBid += bid * quantity;
      _state[auctionId].revealedNonZeroBidCount += quantity;
      _bidTreeByAuction[auctionId].insert(bid, quantity);
    }

If a user has a duplicate bid they will be unable to complete their reveal, causing them to (1) potentially lose an auction they could’ve won and (2) forcing them to only have the option of a no-show deposit claim. The function insert() itself has already been modified to allow for insertions, just this line was left in.

Remediations to consider:

  • Removing the line in the RedBlackTree implementation to allow for modification of a key’s value.
  • Writing a more complete testing suite for modified data structures in the future.
H-8

No guarantee that tokenIds for auction’s ships haven’t been distributed already

Topic
Protocol Design
Status
Impact
High
Likelihood
Medium

Winners of auctioned ships aren’t guaranteed that the ships they have bid on have non-owned tokenIds in the corresponding IShips contract. If a tokenId is accidentally auctioned off twice or duplicated in some other accident, the winner of a ship will not be able to complete ConstructionBay.sol’s claim() function as it will revert during BaseShips.sol’s (or whatever IShips contract is used) issue() function upon the tokenId already existing. This will prevent auction winners from claiming their ships, claiming their rewards from depositing, and reclaiming any part of their deposit which was not part of their bid. Essentially auction winners will have their funds locked in the contract.

Remediations to consider:

  • Keeping track of which ship tokenIds have been promised/distributed already and disallowing auctions which try to re-promise/distribute these same tokens.
H-9

ShipTrait.sol's receiveRandom() will run out of gas when receiving random counts larger than 3

Topic
Input Ranges
Status
Impact
High
Likelihood
High

Currently in ShipTrait.sol, when users try to reveal traits for ships in groups larger than 3 the code will run out of gas and revert. This is because ShipTrait.sol's receiveRandom() function is (1) limited to 100_000 gas total, while (2) will consume at least 27_000 gas per random requested on storage reads/manipulations alone:

function receiveRandom(
    uint256[] calldata randoms,
    uint256[] calldata tokenIds
  ) external onlyRandomOracle {
    uint256 tokenId;
    for (uint256 i = 0; i < tokenIds.length; ) {
      tokenId = tokenIds[i]; // sload = 2_100 gas
      require(
        _revealStateByTokenId[tokenId] == RevealState.PendingSeed, // sload = 2_100 gas
        "Token seed is not pending"
      );

      // Set seedByTokenId
      _seedByTokenId[tokenId] = randoms[i]; // from-zero sstore, 20_000 gas !!!
      // Set reveal ready in mapping
      _revealStateByTokenId[tokenId] = RevealState.ReadyToReveal; // non-zero sstore, 2,900

      unchecked {
        i++;
      }
    }
  }

See Ethereum’s yellow paper for a list of gas-costs per opcode. The gas limit for receiveRandom() is set in RandomOracle.sol’s _requestRandomWords() on line 121:

function _requestRandomWords(
    uint32 wordCount,
    bytes calldata encodedCalldata,
    bool isArray
  ) private returns (uint256) {

        ...

    uint256 requestId = coordinator.requestRandomWords(
      _keyHash,
      subscriptionId,
      3,
      100000, // limit of 100_000 gas per random callback is set here
      wordCount
    );

    ...
  }

Remediations to consider:

  • Setting a higher gas limit in RandomOracle.sol’s _requestRandomWords() (max is 2,500,000) and limiting the length of tokens that can be requested in ShipTrait.sol’s beginReveal() to a number with a gas consumption a margin lower than the limit.
  • Setting the _seedByTokenId[tokenId] variable in beginReveal() to something non-zero in beginReveal(). This will change the 20_000 gas charge for setting a zero storage slot to the gas charge of 2_900 for something non-zero. Overall this approach will cost more gas but you can avoid having to pay the cost in this gas-restricted function where reverts are harmful.
M-1

FxBaseXXXTunnels can have their fxXXXTunnel set by anyone during deployment

Topic
Access Restriction
Status
Impact
High
Likelihood
Low

In FxBaseChildTunnel.sol, lines 54-60, the code which sets the fxRootTunnel for the FxBaseChildTunnel is callable by anyone:

// set fxRootTunnel if not set already
  function setFxRootTunnel(address _fxRootTunnel) external {
    require(
      fxRootTunnel == address(0x0),
      "FxBaseChildTunnel: ROOT_TUNNEL_ALREADY_SET"
    );
    fxRootTunnel = _fxRootTunnel;
  }

Same with FxBaseRootTunnel.sol, lines 71-78, for setting the fxChildTunnel for the FxBaseRootTunnel:

// set fxChildTunnel if not set already
  function setFxChildTunnel(address _fxChildTunnel) public {
    require(
      fxChildTunnel == address(0x0),
      "FxBaseRootTunnel: CHILD_TUNNEL_ALREADY_SET"
    );
    fxChildTunnel = _fxChildTunnel;
  }

This is problematic because the variables fxRootTunnel and fxChildTunnel control which tunnels the deployed protocol’s tunnels will accept messages from. The party which controls the listened to tunnel is able to execute arbitrary code, notably the Dock's ore and ship minting code.

If this initialization code isn’t ran atomically with the deployment of the FxBaseRootTunnel and FxBaseChildTunnel, there exists the possibility of a malicious party submitting their own corresponding tunnel as the ‘valid’ tunnel to receive messages from. This would give the malicious party the ability to mint ore and ships as they please.

With the current deployment code this attack could happen. In the 4_enable_communication.js migrations file, lines 53-61, there is a gap between deploying the tunnels and setting their listened-to tunnel:

async function setFxReflection(Tunnel, reflection) {
    const tunnel = await Tunnel.deployed();

    if (isPolygon) {
      await tunnel.setFxRootTunnel(reflection);
    } else {
      await tunnel.setFxChildTunnel(reflection);
    }

A worst case advanced attack could look like:

  • Malicious user waits for deployment of FxBaseRootTunnel and immediately sets their own FxBaseChildTunnel as the valid root.
  • The deployment team fails to notice this.
  • Malicious user re-creates all of the transactions on their FxBaseChildTunnel that the protocol’s intended FxBaseChildTunnel outputs. Since the FxBaseChildTunnel run on Polygon this wouldn’t be too cost prohibitive.
  • Malicious user waits for the protocol to gain value.
  • Malicious user starts to submit their own transactions to the FxBaseChildTunnel as well, minting themselves the coolest ships or all the ore they want.
  • Malicious user sells ore/ships on open market to make $.

If the advanced user’s tunnel plant is detected, the protocol would need to upgrade the tunnel and change the listened to tunnel’s address to point to the protocol controlled tunnel. This is assuming that the malicious party hasn’t minted anything yet.

Remediations to Consider

  • (1) Restricting who can call setFxXXXTunnel() in both FxBaseChildTunnel.sol and FxBaseRootTunnel.sol, or
  • (2) Deploying the FxBaseXXXTunnel and setting the fxXXXTunnel from a transaction atomically.
M-2

setSale() does not check for the startTime

Topic
Input Ranges
Status
Impact
High
Likelihood
Low

There is not a check for the startTime of the sale in the setSale()in DutchAuctionShips.sol. If a time in the past gets passed into this function, the auction may start from an unexpectedly low price, letting early users mint all ships.

Remediations to Consider

  • Requiring startTime to be bigger than the current timestamp.
M-3

Dock tunnels allow governance to make arbitrary calls

Topic
Extra Code
Status
Impact
Medium
Likelihood
Medium

The tunnels defined in StandardChildTunnel.sol and StandardRootTunnel.sol have extra code which allows the governance to run arbitrary code:

/**
 * @notice Make arbitrary calls on behalf of this tunnel
 * @dev Used by governance when docking calls fail
 * @param targets The addresses of the contracts to call
 * @param values The currency amounts to send with calls
 * @param calldatas The calldata for every call, like function signatures and parameters
 */
function replayCalls(
  address[] calldata targets,
  uint256[] calldata values,
  bytes[] calldata calldatas
) external onlyRole(REPLAYER_ROLE) {
  _execute(targets, values, calldatas);

  emit CallsReplayed(targets, values, calldatas);
}

This isn’t a part of the yellow or white paper and is an extra feature that expands the attack surface of the protocol.

Remediations to Consider

  • Removing this code.
M-4

Ships are locked when belt corruption or theft percents are set to > 100%

Topic
Asset Mobility
Status
Impact
High
Likelihood
Low

In the RewardsLib.sol’s publishBelt(), there is no check if _corruption or CrimeOutcome.tax are set to a value greater than 100% (10_000).

Consider the following scenario after a belt was successfully published with one of these values set > 10_000:

  1. The pilot (with staked ship) calls beginMining which sets ShipState to Mining.

  2. The pilot calls completeMining which sets ShipState to Claiming.

  3. Finally, if pilot wants to claim their rewards by calling revealMiningClaim, the transaction reverts if corruption or randomly chosen theft percent > 100% due to an underflow on the following line (RewardsLib.sol, line 248):

    uint256 oreLost = (oreMined * tax) / FIXED_POINT_BASIS;
    **uint256 oreRewarded = oreMined - oreLost**         // underflow
    

Consequently, the ship is forever locked in Claiming state, and because of that, the pilot cannot do any further transactions.

Remediations to Consider

  • Confirming _corruption and all _crimeOutcomes.tax values are less than or equal to 10_000 before publishing the belt.
M-5

Ships in the upgrading process can be unstaked

Topic
Input Validation
Status
Impact
Medium
Likelihood
Medium

Players who beginUpgrade() can unstake() the ship before the upgrade is completed, which will result in losing the cost paid for the upgrade. If the player tries to re-stake their ship, although there is an ongoing upgrade, they won’t be able to call completeUpgrade() as the state of the ship is not Upgrading anymore. This is simply because Game.sol only checks if the ship is in the Citadel before unstaking it and does not check what is the current state() of the ship.

Remediations to Consider

  • Require the ship to be not in Upgrading state in unstake()
M-6

issue() and update() do not check for unique ship names

Topic
Spec
Status
Impact
Spec Breaking
Likelihood
Medium

issue() and update() in the BaseShips.sol are not checking the uniqueness of the ship names, and also ship names added by these functions won’t be registered in doesExistByName.

These functions are implemented to get used by the Dock, but it is mentioned that governance will also use these functions for various reasons, including renaming ships. Names set by the governance using these functions can be double registered by players later. Furthermore, if the governance passes an already used name, it will result in duplicate names.

Remediations to Consider

  • Implementing a new function specific for governance use and applying the same requirements as setName()
M-7

Punishment order may affect system revenue and create adverse incentives

Topic
Incentive Design
Status
Impact
Medium
Likelihood
Medium

In ConstructionBay, anyone can call punish function to charge tax on bidders who didn’t reveal their bids on time. In case their bids were winning bids and not all available ships in the auction have been claimed there is an additional tax - no-show tax. Currently no-show tax, which is significantly larger than normal tax (50% vs 5%), is charged only for the first noShowShipCountByAuction. Every other bidder after that will be charged only flat tax fee.

While the intention of the current implementation was to not charge the no-show penalty to more bidders than could have claimed during the auction, its side effect is that no-show bidders with winning bids, depending on the punishment order, will be charged significantly different tax amounts and also tax revenue for the system may vary depending on which bidders are charged.

Remediations to Consider

  • Consider charging no-show tax to all bidders with winning bids who didn’t reveal their bids on time, in the case when not all available ships have already been claimed, to avoid introduction of adverse incentives.
M-8

No guarantee auctioned ships have existing or useable IShipTraits contract

Topic
Protocol Design
Status
Impact
Medium
Likelihood
Medium

Winners of auctioned ships aren’t guaranteed that the ships they’ve won can be revealed as the ships’ corresponding IShipTrait contract could (1) not exist or (2) not have the uploads ready/complete.

Remediations to consider:

  • Checking that the IShipTraits contract is valid and has ship uploads enabled.
M-9

ShipTraits.sol: possible to be unable to assign traits to a token range

Topic
Spec
Status
Impact
Medium
Likelihood
Medium

In ShipTraits.sol’s batch creation process it is possible to create token ranges which cannot be included in a batch. This can happen if a token range is skipped in the batch creation process.

In ShipTraits.sol’s createBatch() function, there exists a requirement that the starting token in the batch’s range is larger than the previous’s batch’s last token:

require(
  batchId == 1 || (params.startTokenId > _params[batchId - 1].endTokenId),
  "Invalid token range"
);

This is problematic if a batch accidentally skips tokens. The skipped tokens will not be able to be included in another batch as they will fail to meet the above requirement. This will cause there to be ships which are unable to be assigned traits.

Remediations to consider:

  • Requiring that each created batch leave no gaps between its token range and the previous batch’s token range.
M-10

ShipTraits.sol’s _selectShip() will revert if no normal ships are uploaded

Topic
Input Ranges
Status
Impact
Medium
Likelihood
Medium

ShipTraits.sol’s _selectShip() will revert if no normal ships are uploaded due to a modulus-by-zero error during the ship selection process:

if (seed < officerRarity && isOfficerAvailable) {
  c = _uploads[batchId].officerShips[seed % officersLength];
} else {
  // Select ship combination with Alias method
  uint128 rarityIndex = uint128(seed) %
    uint128(_uploads[batchId].rarityPairs.length); // will revert here
  RarityPair memory rarityPair = _uploads[batchId].rarityPairs[rarityIndex];
  uint128 index = seed >> 128 < rarityPair.rarity
    ? rarityIndex
    : rarityPair.aliaz;
  c = _uploads[batchId].normalShips[index];
}

This will cause people who have acquired ships to be unable to finish the reveal process.

Remediations to consider:

  • Not allowing batches to be considered valid if no normal ships are uploaded.
M-11

ShipTraits.sol’s validateData() will revert if no officer ships are uploaded but some are expected

Topic
Input Ranges
Status
Impact
Medium
Likelihood
Low

ShipTraits.sol’s validateData() function will revert if no officer ships are uploaded but some are expected during the emit of the ValidationFailed event:

if (!isValid) {
  _state[batchId].status = BatchStatus.Uploading;
  delete _uploads[batchId];
  delete _state[batchId].unvalidatedIndex;

  emit ValidationFailed(batchId, i - 1); // this will cause underflow
}

The index at this point will be equal to zero so subtracting 1 will cause a revert. This will cause the incorrectly uploaded batch to be stuck in the BatchStatus.Validating state.

Remediations to consider:

  • Changing the emit event to not cause a revert when i is equal to zero.
M-12

Extreme values for the revealPeriodDuration parameter may result in unexpected tax charges for auction participants or lead to an effectively locked ConstructionBay contract due to missing range check

Topic
Input Ranges
Status
Impact
High
Likelihood
Low

In ConstructionBay, the auction is created in addAuction function callable by admin. In this function the revealPeriodDuration parameter is checked to be ≠ 0. However, since there is no lower bound or upper bound on the value of this parameter, unexpectedly small or large values may negatively impact the overall function of the ConstructionBay.

When revealPeriodDuration is extremely small (e.g. 1) auction participants who committed their bid to the auction will not have enough time to reveal their bid. As a result, from the system's perspective, they will be treated as a no-show auction participant and required to pay a no-show tax penalty.

When revealPeriodDuration is extremely large (e.g. 31536000 - number of seconds in 1 year), a particular auction will not end (get to Claiming phase) until that period passes. What exacerbates this issue is that the next auction cannot be added until the previous one ends due to the following check in the _maximumPromisedToken function. Effectively contract will be locked for the period defined in revealPeriodDuration.

require(
  status(lastAuctionId) == AuctionStatus.Claiming,
  "Last auction is not complete"
); 

Remediations to Consider

  • Add a range check for the revealPeridDuration parameter with sensible min and max values.
M-13

Auction startTime can be in the past, while endTime can be too large

Topic
Input Ranges
Status
Impact
High
Likelihood
Low

In ConstructionBay, the startTime for auction is checked to be ≠ 0 and to be less than the endTime, but not that is in the future. On the other hand, endTime doesn’t have specific validations, and by accident, extremely large values can be provided as a result.

When startTime is in the past depending on the other parameters (primarily endTime and revealPeriodDuration) auction may end up in a period where reveal bids are expected to be submitted even though due to the value of the same parameters no one was able to previously bid. As a result, no actions on the ConstructionBay contract will be possible until the revealPeriodDuration period passes and the auction ends up in AwaitingWinningBid phase.

When endTime has an extremely large value impact would be similar to when revealPeriodDuration value is too large - deposited assets of auction participants would be locked for a specified period of time and no new auctions would be possible to add due to the following guard in the _maximumPromisedToken function which prevents adding new auctions unless previous has completed.

require(
  status(lastAuctionId) == AuctionStatus.Claiming,
  "Last auction is not complete"
);

Remediations to Consider

  • Add validation for startTime to be in the future.
  • Add validation for endTime to be in an expected range from startTime.
M-14

DutchAuctionShips.setSale() can be called multiple times leading to unexpected behavior

Topic
Spec
Status
Impact
Medium
Likelihood
Low

In DutchAuctionShips, the setSale function can be called multiple times by the sale admin and as a result, the sale record can be changed/configured multiple times. This potentially could lead to auction behavior that is radically different than one considered to be a Dutch auction. For example, due to updates in subsequent invocations the price can increase even though it is expected to decrease over time.

Remediations to Consider

  • Consider not allowing setSale invocation if the sale record has already been updated.
M-15

_maximumPromisedTokenId() can return an incorrect result

Topic
Spec
Status
Impact
Medium
Likelihood
Low

In ConstructionBay, in the coreTeamClaim function startTokenId is generated in the following way:

**uint256 communityShipCount = _parameters[auctionId].communityShipCount;**
uint256 coreTeamShipCount = _parameters[auctionId].coreTeamShipCount;
uint256 coreTeamClaimedCount = _state[auctionId].claimedCoreTeamCount;

...

uint256 startTokenId = _parameters[auctionId].startTokenId +
  communityShipCount +
  coreTeamClaimedCount;

On the other hand, the result of _maximumPromisedTokenId(), when the particular auction is not the first ConstructioBay auction, is calculated in the following way:

uint256 maximumIssuedTokenId = ships.maximumTokenId();
...
uint256 startId = _parameters[lastAuctionId].startTokenId;
**uint256 communityCount = _state[lastAuctionId].eventualCommunityClaimCount;**
uint256 coreTeamCount = _parameters[lastAuctionId].coreTeamShipCount;

return
  MathUpgradeable.max(
    maximumIssuedTokenId,
    startId + communityCount + coreTeamCount - 1
  );

Notice that here code relies on the eventualCommunityClaimCount value which may be less than the communityShipCount value used in the coreTeamClaim function calculation above. As a result, _maximumPromisedTokenId may return a result that is smaller than the maximum promised token id for the previous auction.

Remediations to Consider

  • Make these two calculations consistent by updating coreTeamClaim function or _maximumPromisedTokenId so they rely on the same inputs.
M-16

_maximumPromisedTokenId() check can be ineffective

Topic
Spec
Status
Addressed
Impact
High
Likelihood
Low

In ConstructionBay, the _maximumPromisedTokenId() function, as its name says, is responsible for returning the maximum promised token id which is used to prevent double selling the same tokenId. This check is necessary so that ship traits corresponding to a particular range of token ids get properly assigned.

However, if there is an overlap between auctions created in ConstructionBay and auctions defined by FixedPriceShips/DutchAuctionShips (they have not been completed or they are running in parallel) which also have the capability to mint ships this check may pass even though particular tokenId may later become unavailable to a buyer who obtained it through ConstructionBay auction.

Remediations to Consider:

  • Ensure that FixedPriceShips/DutchAuctionShips are completed and never used after transitioning to ConstructionBay auctions.
Response by Citadel

Addressed - Only a single instance of DutchAuctionShips will exist, and its sale will complete before any ConstructionBay auctions start.

M-17

Small value for requestConfirmations parameter allows block miners to increase their chances to mine more valuable ships

Topic
3rd Party Behavior
Status
Impact
Medium
Likelihood
Low

In the RandomOracle._requestRandomWords() function the value of requestConfirmations parameter is hardcoded to 3 which is the default minimum for the Polygon chain:

uint256 requestId = coordinator.requestRandomWords(
  _keyHash,
  subscriptionId,
  3,
  2500000,
  wordCount
);

In this particular blockchain environment chain reorgs of length greater than 3 blocks are more frequent (see here https://polygonscan.com/blocks_forked).

Due to the above, block miners may find it lucrative enough to perform a chain reorg in order to increase the chances of revealing more valuable ships, as they can re-request new random values. For more details check: https://docs.chain.link/vrf/v2/security#choose-a-safe-block-confirmation-time-which-will-vary-between-blockchains.

Increasing requestConfirmations value results in an increased period of time that users will need to wait between initiating beginReveal and being able to perform completeReveal. Since the block generation period in the Polygon chain is approximately 2s, the current value indicates that the waiting period is at least 10s. Changing it to e.g. 50 would increase that waiting period to almost 2 mins.

Remediations to consider:

  • Increase requestConfirmations parameter value to one which represents the appropriate balance for Citadel’s use case.
L-1

Late revealers may miss the chance of minting an officer ship

Topic
Input Validation
Status
Wont Do
Impact
Low
Likelihood
Low

The chance of revealing a pre-made officer ship is calculated as ~1% inGenesisShipTraits.sol and there is a fixed number of officer ships. If the ratio of officer ship count to total supply is less than 1%, late revealers won’t have a chance to mint an officer ship statistically because once the fixed number of ships is revealed _traitsOfTokenWithSeed() will prevent minting more

Remediations to Consider

  • Requiring the length of officerShips_ is greater or equal to 1% of token count(endTokenId - startTokenId) in GenesisShipTraits.sol’s initialize()
Response by Citadel

Wont do - This is intentional

L-2

Case-sensitivity makes the unique name and vulgar name mechanics obsolete

Topic
Spec
Status
Impact
Spec Breaking
Likelihood
High

Ship names are stored as bytes in BaseShips.sol:L440 and they are case-sensitive. Users can use vulgar words by just typing some letters in upper-case or lower-case. Since blacklisting all combinations in addBannedNames() won’t be possible because of the huge number of different combinations, the approach taken to block vulgar names seems easily nullable.

The same issue is also valid for the uniqueness of the ship names.

Remediations to Consider

  • Converting all letters to lowercase before storing on-chain.
  • Removing the on-chain blacklist of names and blocking vulgar names on the client side. Vulgar names registered by directly interacting with the contract can be updated(and perhaps penalized) using the update() function.
L-3

setSale() does not check for maxPublicPerWallet > 0

Topic
Input Validation
Status
Impact
Low
Likelihood
Low

In DutchAuctionShips.sol’s setSale function, there is no check that maxPublicPerWallet in Sale struct must be greater than 0. If maxPublicPerWallet is set to 0, nothing can be minted and minting will revert with “Can't mint more than the max”.

Remediations to Consider

  • Adding a check for maxPublicPerWallet > 0. Alternatively, adding an option to treat maxPublicPerWallet == 0 as without limit minting.
L-4

No boundary checks for daoTaxSharePercentage in addAuction

Topic
Input Validation
Status
Impact
High
Likelihood
Low

In ConstructionBay.sol’s addAuction function, there is no upper check for the daoTaxSharePercentage parameter. If this parameter is set to a higher value as 10_000 (100%), than the revealBidOnTime call would fail with an underflow on the following line:

stateByAuction[auctionId].token.burnFrom(
        address(this),
        spentAmount - daoPortion
      );

As a result, winning bidders cannot claim their ships and don’t get any remaining balance back (except the balance which is leftover after punishing).

Remediations to Consider

  • Consider adding a check so that daoTaxSharePercentage cannot be greater than 10_000.

Note: The other tax parameters

  • flatTaxPercentage
  • noShowTaxPercentage

could possibly have a value greater than 10_000.

L-5

Officer ship owners can choose which ship to reveal

Topic
Randomness
Status
Impact
Medium
Likelihood
Low

GenesisShipTraits.sol uses a fixed array allOfficerShips to store all possible officer ships. This array is later used to assign officer ship traits to lucky pilots. The process is supposed to be random, but since the revealing order affects which ship will be revealed, a user can figure out they own an officer ship before revealing it and can choose which officer ship they will reveal by observing the order in the array and queued reveals.

The seeds that match the following conditions will reveal to an officer ship:

  • Least significant byte % 4 == 3
  • Second least significant byte < 10

Once such seed is returned from the oracle, a user can start watching for completeReveal() transactions of other officer ship revealers and front-run the one they want to reveal for themselves.

Remediations to Consider

  • Changing the design to a synchronous reveal based on random numbers.
L-6

LINK can be locked in a deprecated RandomOracle

Topic
Asset Mobility
Status
Impact
Low
Likelihood
Medium

If a new RandomOracle implementation is deployed — for example, in order to use a newer VRFCoordinator — there is no way to transfer any unused LINK from the deprecated RandomOracle.

Remediations to Consider

  • Updating RandomOracle.sol to be upgradeable: the balance remains with the proxy, avoiding the need for transfer. Otherwise consider implementing a transfer LINK feature with appropriate privileges.
L-7

Unrevealed on-sale ships can be sniped

Topic
Design
Status
Acknowledged
Impact
Low
Likelihood
Medium

Revealing ships is a two-step process. Users request a random number calling beginReveal() and once the random number arrives they make a second call to completeReveal(). Although the ship is not revealed until the second tx completes, malicious users can check the random number the moment oracle calls back. Using that number as a seed, traits of the ship can be calculated and the user can buy on-sale ships with high rarity even before they are revealed.

Remediations to Consider

  • Warning users on the client side about the dangers of revealing an on-sale ship when they start a reveal.
  • Adding this info to the documentation of the game.
Response by Citadel

Acknowledged - Will document on website

L-8

Ships are locked when belt is not published

Topic
Asset Mobility
Status
Impact
High
Likelihood
Low

In the Game.sol contract, adding a belt is a 2-step process:

  1. addBelt

  2. publishBelt

According to yellowpaper, adding and publishing belts retains the responsibility of the Citadel team even after control was handed over to the DAO:

Practically, this means that The Citadel team will retain only the roles necessary to add belts, publish belts, and improve gameplay, while still allowing the governance system to revoke each of these permissions (and not vice versa)

Once Citadel team adds a new belt via addBelt function, pilots can already begin mining or roaming on the newly added belt. However, pilots cannot complete mining before the belt has been published by the Citadel team due to the following check in RewardsLib.sol, line 148:

require(isBeltPublished(beltId), "Belt is not published");

As a consequence, ships already mining on not-published belts are locked in the Mining state until Citadel team decides to publish the belt. This gives the Citadel team control over other players as team can decide when to publish the belt, or even don’t publish the belt at all.

Remediations to Consider

  • Letting pilots mine or roam only when belts are published.
  • Allow miners to stop mining non-published belts with no rewards
L-9

Proofs cannot have auctionIDs higher than uint248_max while ConstructionBay.sol can

Topic
Input Ranges
Status
Impact
Low
Likelihood
Low

The BidTicketHasher() circom component has max signal size of 248 bits. Currently ConstructionBay.sol’s accepted auctionID range is larger at 256 bits. Auctions added with auctionIDs larger than 248 bits will not be able to have anyone bid on them as no one will be able to generate valid proofs.

Remediations to consider:

  • Limiting ConstructionBay.sol’s auctionID AuctionParameters field to be no greater than 248 bits wide.
L-10

Missing validation on AuctionParameters’s IERC20Burnable token parameter

Topic
Input Validation
Status
Impact
Low
Likelihood
Low

Auctions can be created that have the zero address for their payment token contract address. No funds are at risk but the created auction would be unusable.

Remediations to consider:

  • Checking that the token parameter is not equal to zero.
L-11

Users can bid at the auction even if communityShipCount is zero

Topic
Use Cases
Status
Impact
Low
Likelihood
Low

In ConstructionBay, users can bid at the auction even if communityShipCount is zero. So in order to reclaim their funds they will be charged flatTaxPercentage even though they had not been able to claim any ships in the first place.

Remediations to consider:

  • Add a guard in ConstructionBay#commit to prevent users from submitting bids to auction when they will not be able to claim any ships.
L-12

BaseShips.sol’s resetName() effects can fail to stay consistent between layers

Topic
Spec
Status
Impact
Low
Likelihood
Medium

The way in which BaseShips.sol’s resetName() interacts with Dock.sol’s undock() can result in a name reset not holding when a ship is docked on polygon. The name reset can also cause the naming system’s accounting to become out of sync.

Currently, undock() will update a ship’s name on the new dock using the name sent over from the other dock:

function undock(
  uint256[] calldata tokenIds,
  Ship[] calldata traits,
  bytes32[] calldata names,
  address to
) external onlyTunnel {
   ...

  for (uint256 i = 0; i < tokenIds.length; i++) {
    bool exists = ships.exists(tokenIds[i]);
    require(
      !exists || ships.ownerOf(tokenIds[i]) == address(this),
      "This ship is issued to someone else"
    );

    if (exists) {
      // ship is updated with other layer's reported name
      ships.update(tokenIds[i], traits[i], names[i]);
      ships.transferFrom(address(this), to, tokenIds[i]);
    } else {
      ships.issue(to, tokenIds[i], traits[i], names[i]);
    }

    emit ShipUndocked(tokenIds[i], traits[i], to);
  }
}

Let’s explore the following scenario:

  • ShipA is owned by the user on L1 with the name ‘shipA’, and ShipA’s representation on the L2 is owned by the dock.
  • Governance changes ShipA’s name on L2 to ‘newNameShipA’, performing all of the name-updating and deletion logic, including setting the doesExistByName entry for the name to false on the L2.
  • The user transfers ShipA from L1 to L2, resetting the name of ShipA on the L2 back to ‘shipA’.
  • Currently:
    • L1 has doesExistByName[shipA] as true and doesExistByName[newNameShipA] as false
    • L2 has doesExistByName[shipA] as false and doesExistByName[newNameShipA] as true
  • Issues with the above accounting status:
    • A user can name their ShipB ‘shipA’ on the L2 and now two ships have the same name
    • No ship has the name ‘newNameShipA’ but the name is registered as taken on the L2

Remediations to consider:

  • Only allow name changes to occur when a ship is not docked on the L2 and have BaseShips.sol’s _storeName() function also update the naming system’s doesExistByName accounting map instead of _setName().
L-13

ConstructionBay's _coreTeamSplit variable is never set in the migrations scripts

Topic
Spec
Status
Impact
Low
Likelihood
High

ConstructionBay's _coreTeamSplit variable is never set in the migrations scripts to the desired 15% as specified in the documentation.

Remediations to consider:

  • Setting the _coreTeamSplit variable to the desired 15%.
L-14

L2’s ShipTraits's DEFAULT_ADMIN_ROLE is not relinquished or transferred to governance in migration scripts

Topic
Spec
Status
Impact
Low
Likelihood
High

In 5_relinquish_to_governance.js and in 3_deploy_contracts.js, L2’s ShipTraits's DEFAULT_ADMIN_ROLE is not transferred or relinquished to governance. This is against spec.

See that 5_relinquish_to_governance.js skips the contract’s access control transfer on line 129:

await relinquish(ShipTraits, {
  hasAccessControl: false,
  grantUpgradesToMultisig: true,
});

Remediations to consider:

  • Properly relinquishing control of the ShipTraits contract to governance.
L-15

citadel-config.js’s coreTeamAddress and coreTeamMultisigAddress are the same

Topic
Spec
Status
Acknowledged
Impact
High
Likelihood
High

In citadel-config.js the coreTeamAddress and coreTeamMultisigAddress are the same. This goes against spec.

Note: this could be due to the multisig not being setup yet or for testing purposes.

Remediations to consider:

  • Changing the variables to reflect the actual addresses which should not be the same.
Response by Citadel

Acknowledged - We haven’t deployed a multisig yet

L-16

Core Team does not receive 15% of the initial ship supply

Topic
Spec
Status
Impact
High
Likelihood
High

In citadel-config.js the initial supply for the DutchAuctionShips contract is set at:

  supply: {
    publicSupply: 1583,
    whitelistSupply: 1583,
    coreTeamSupply: 167,
  },

This results in the core team receiving 5% of total ships for free instead of the spec defined 15%.

Remediations to consider:

  • Updating citadel-config.js to give the proper 15% of ships.
L-17

Citadel payment splitter is doing the wrong core/governance split

Topic
Spec
Status
Impact
High
Likelihood
High

In citadel-config.js the payment splitter ratio is set at 50:50 instead of the spec defined 80:20 split with the governance receiving more.

Remediations to consider:

  • Changing the citadel-config.js values to the proper 80:20 split.
L-18

ticketHash collisions are possible

Topic
Spec
Status
Impact
Low
Likelihood
Low

In ConstructionBay.sol, users can have ticket hash collisions in their commit transactions as none of the information in the Circom circuits is required to be unique to the user. Consider the scenario where first Alice and then Bob submit bids to an auction with proof inputs: { auctionId : 1, bid: 1000, quantity: 1, secret: 1, usableDepositAmount: 1000}. Bob’s commit transaction will fail to go through and he will know what Alice’s bid is if he decides to inspect etherscan.

Remediations to Consider:

  • Adding the public key of the msg.sender to the commit circuit and pull the variable from the msg.sender variable during commit() ’s verification of the proof. This will remove the possibility of users accidentally having ticketHash collisions.
Q-1

Wrong parameter type

Topic
Code Quality
Status
Quality Impact
Low

DutchAuctionShips.sol:L216 _seconds() and _wei() are expecting uint64 params, but they are always called with uint32 params.

Q-2

fulfillRandomness() should not revert

Topic
Code Quality
Status
Quality Impact
Medium

per Chainlink docs:

If your fulfillRandomness implementation reverts, the VRF service will not attempt to call it a second time. Make sure your contract logic does not revert.

The following methods are called from RandomOracle.sol::fulfillrandomness() and can potentially revert:

RewardsLib.sol::writeMiningRandom():

function writeMiningRandom(uint256 _random, uint256 tokenId)
  external
  onlyRandomOracle
{
  require(_isPendingByTokenId[tokenId], "Random is not pending");
  require(_randomByTokenId[tokenId] == 0, "Random already written");
...

BaseShipTraits.sol::receiveRandom():

function receiveRandom(uint256 random, uint256 tokenId)
  external
  onlyRandomOracle
{
  require(_isPendingByTokenId[tokenId], "Token seed is not pending");

These reverts will in turn cause fulfillrandomness() to revert, per RandomOracle.sol lines 80-81:

string memory errorMessage = "RandomOracle: call reverted without message";
Address.verifyCallResult(success, data, errorMessage);

Consider updating fulfillrandomness() to emit an event in these scenarios rather than reverting.

Q-3

Reveal-state tracking in BaseShipTraits.sol can be simplified

Topic
Code Quality
Status
Quality Impact
Medium

Presently, multiple variables are used in tracking the reveal state of a token:

  • _isPendingByTokenId
  • _isRevealedByTokenId
  • _seedByTokenId

The reveal flow can more cleanly be tracked in a single state variable in conjunction with an enum. e.g.:

enum RevealState {
  None,
  PendingSeed,
  ReadyToReveal,
  Revealed
}

mapping(uint256 => RevealState) private _revealStateByTokenId;
~~mapping(uint256 => bool) private _isPendingByTokenId;~~
mapping(uint256 => uint256) private _seedByTokenId; // still used, but not for state-tracking
~~mapping(uint256 => bool) private _isRevealedByTokenId;~~

In lieu of using _isPendingByTokenId, _isRevealedByTokenId, and _seedByTokenId to check state, subsequent logic can be simplified:

beginReveal():

  • _revealStateByTokenId[tokenId] must be RevealState.None, and is then set to RevealState.PendingSeed

receiveRandom():

  • _revealStateByTokenId[tokenId] must be RevealState.PendingSeed, and is then set to RevealState.ReadyToReveal

completeReveal():

  • _revealStateByTokenId[tokenId] must be RevealState.ReadyToReveal, and is then set to RevealState.Revealed
  • _seedByTokenId[tokenId] can be deleted (this variable is no longer used for state-tracking)

isPending():

  • returns true when _revealStateByTokenId[tokenId] is RevealState.PendingSeed

isRevealed():

  • returns true when _revealStateByTokenId[tokenId] is RevealState.Revealed

Note that there is currently no view function corresponding to the RevealState.ReadyToReveal state. Consider adding one, or replacing isPending() and isRevealed() with a generic function which returns the RevealState for a token (e.g. function getRevealState(uint256 tokenId) public view returns (RevealState) { ... })

Q-4

Use of transfer function

Topic
Code Quality
Status
Quality Impact
Medium

In _refundIfOver in DutchAuctionShips.sol, the transfer function is used to send back remaining ether to caller. Consider to use AddressUpgradeable.sendValue (see here) instead, as transfer function is not recommended to use anymore. Detailed explanation on why transfer shouldn’t be used anymore can be found here.

Q-5

Deployer retains REQUESTER_ROLE for RandomOracle

Topic
Code Quality
Status
Quality Impact
Medium

See RandomOracle.sol line 33, within the constructor:

_grantRole(REQUESTER_ROLE, msg.sender);

If it is desirable that an EOA retains this feature, consider transferring this role to governance; otherwise consider removing the grant.

Q-6

Use _msgSender() instead of msg.sender in OpenZeppelin contract extensions

Topic
Code Quality
Status
Quality Impact
Low

In ERC20Locked.sol, the modifier onlySupplyAdmin() uses msg.sender instead of _msgSender() which is defined in the inherited ContextUpgradeable contract. This isn’t a security vulnerability because no account forwarding is used, but, code uniformity is nice.

modifier onlySupplyAdmin() {
  require(_isSupplyAdmin(msg.sender), "Only supply admins supported");
  _;
}

Consider: using _msgSender() instead of msg.sender.

Q-7

Unnecessary output signal in Reveal circuit

Topic
Extraneous Code
Status
Quality Impact
Medium

In reveal.circom, the totalBid output signal is not used in any meaningful way and provides no value. Its information is already constrained in the bid and quantity’s use in the ticketHash.

Remediations to consider:

  • Removing this variable from the circuit and proof.
Q-8

AuctionAdded event missing indexed attribute for auctionId parameter

Topic
Events
Status
Quality Impact
Medium

Add an indexed attribute to auctionId parameter in AuctionAdded event declaration for easier off-chain tracking and monitoring.

Q-9

Unnecessary cast in _isCommitProofValid

Topic
Extraneous Code
Status
Quality Impact
Low

ticketHash which is a uint256 argument to _isCommitProofValid is cast unnecessarily.

pubSignals[0] = uint256(ticketHash);
Q-10

A misleading error message in BaseShips.sol

Topic
Revert Messages
Status
Quality Impact
Low

Error message at BaseShips:L638 is misleading. If the amount + minted count is bigger than the team supply that does not necessarily mean all tokens are minted. Consider changing this message to include cases where a wrong amount is passed to the function. The same issue is present with other mint functions in BaseShips.

Q-11

Unnecessary check in ShipTraits.beginReveal function

Topic
Extra Code
Status
Quality Impact
Low

In the beginReveal function, _seedByTokenId[tokenId] == 0 check is unnecessary since it will be always satisfied when the other part of the condition _revealStateByTokenId[tokenId] == RevealState.None is satisfied.

require(
  _seedByTokenId[tokenId] == 0 &&
    _revealStateByTokenId[tokenId] == RevealState.None,
  "Reveal already pending"
);
Q-12

Ships SUPPLY_ADMIN_ROLE is overloaded

Topic
Access Control
Status
Quality Impact
Medium

Use a different permission role, other than SUPPLY_ADMIN_ROLE, to control access to functions in the BaseShips contract (issue and update with name parameter) which are used only by the Dock contract (bridge).

Q-13

BaseShips.resetName function is not defined in the IShips interface

Topic
Interfaces
Status
Quality Impact
Medium

Interfaces should define all external functions that are implemented in implementation contracts.

Q-14

Interface functions do not have expected natspec documentation

Topic
Comments
Status
Quality Impact
Medium

To be consistent across the codebase define natspec documentation on the interface level so that documentation can be automatically inherited (if other rules are respected) by the implementation.

Q-15

Parameter name differs in function declaration and function implementation

Topic
Interfaces
Status
Quality Impact
Low

In the IShips interface, functions exists, traits, nameAsBytes32, nameAsString, setName are declared with shipId parameter, while implementation of the same functions in BaseShips.sol uses a parameter named tokenId. This prevents the automatic inheritance of natspec documentation that is defined on the interface level (see Inheritance notes for NatSpec format).

Q-16

Add an override keyword to the interface function

Topic
Interfaces
Status
Quality Impact
Low

Add override keyword to functions which are implementation of functions declared in parent interfaces to more explicitly differentiate interface-related functionality from contract-specific functionality. For example, add an override keyword to functions in the BaseShips contract which are implementations of corresponding functions declared in the IShips interface.

Q-17

Over granting of Oracle’s REQUESTOR_ROLE

Topic
Access Control
Status
Quality Impact
Medium

In 3_deploy_contracts.js the DutchAuctionShips contract is given random oracle permissions when the contract does not need them

// Set up random oracle requesters
const randomRequesters = [ships, rewards, shipTraits];
await grantRole(oracle, randomRequesters, "REQUESTER_ROLE", { step });
Q-18

Unneeded governancePercentage hardcode value

Topic
Extra Code
Status
Quality Impact
Low

In 3_deploy_contracts.js the child’s Ship contract has the governancePercentage variable hardcoded to 10_000. This is the correct value but it is weird that it is hardcoded when it could be read from the citadel-config.js file instead.

Q-19

Documentation improvements

Topic
Comments
Status
Quality Impact
Low
  • The following comment for BaseShips.update(uint256 tokenId, Ship calldata ship) is incorrect since this function is not called by the dock. It is called from the Game and ShipTraits contract.
@dev This functionality is used by the dock and the ship traits generators
  • The following comment for BaseShips.update(uint256 tokenId, Ship calldata ship, bytes32 name) is incorrect since this function is only called by the dock.
@dev This functionality is used by the dock and the ship traits generators
  • The following comment for BaseShips.function issue(address to, uint256 id, Ship calldata ship) is incorrect since this function is only used by the ConstructionBay.
@dev This functionality is used by the dock and the construction bay
  • The following comment for BaseShips.issue(address to, uint256 id, Ship calldata ship, bytes32 name) is incorrect since this function is only used by the dock.
@dev This functionality is used by the dock and the construction bay
  • In IConstructionBay, the natspec comment describing the WithdrawalProcessed event is incorrect
/// @notice Emitted when a depositor claims a refund for a neglected auction
event WithdrawalProcessed(uint256 indexed auctionId, uint256 amount);
  • In IConstructionBay, the natspec comment describing the revealPeriodDuration struct field misses an expected unit (e.g. - in epoch seconds).
/// @notice The window after `endTime` during which the winning bid may be submitted
uint256 revealPeriodDuration;
  • In IConstructionBay, the natspec comments describing hasClaimed and ticketHash struct fields are incorrect.
/// @notice The token wei they've claimed in depositor rewards
bool hasClaimed;    
/// @notice The hash of the bid ticket and secret
uint256 ticketHash;
  • In IShipTraits, the natspec comment misses an entry for isComplete parameter
/**
* @notice Upload a list of ship trait combinations and weights to be used for generation
* @param batchId The token batch to upload for
* @param uploads The list of officer and normal ship combinations to upload, along with their rarities and aliases
*/
function uploadData(
  uint256 batchId,
  BatchUploads calldata uploads,
  bool isComplete
) external;
  • In IConstructionBay, natspec comments for multiple functions miss parameter and return value descriptions.
Q-20

Commit circuit can become under-constrained if the allowed quantity limit increases

Topic
Information
Status
Quality Impact
High

In commit.circom, the check that usableDepositAmount is equal to or greater than the minimum amount is under-constrained and could have been circumvented if a follow-up check which limits the size of the quantity input was not present.

component amountComparator = LessThan(248);
signal minimumAmount;
minimumAmount <== bid * quantity;
amountComparator.in[0] <== usableDepositAmount;
amountComparator.in[1] <== minimumAmount;

amountComparator.out === 0;

The following set of inputs would satisfy LessThan circuit constraints even though bid * quantity product, as a 254-bit number, is much larger than provided usableDepositAmount.

INPUT = {
  "usableDepositAmount": "1000000000000000000",
  "bid": "83734101653343784507316724941668312298814564542249515196953410152519132161",
  "quantity": "256"
}

LessThan circuit, constraints the result to be a 249-bit number. However, it doesn't constrain inputs to 248-bit numbers. As a result, an attacker may provide suitable inputs which would cause underflow and satisfy the final check.

template LessThan(n) {
  assert(n <= 252);
  signal input in[2];
  signal output out;

  component n2b = Num2Bits(n+1);

    // n2b.in must be 249-bit number
  // but in[0] and in[1] are not checked
  // if they are 248-bit numbers
  n2b.in <== in[0]+ (1 << n) - in[1];

  out <== 1-n2b.out[n];
}

This issue is not currently exploitable because minimumAmount needs to be a 254-bit number in order to exploit it. Bid and quantity can at max be 248-bit numbers due to additional constraints present in the rest of the circuit. Therefore the product of the bid and quantity could be a 254-bit number if there was not a constraint that the quantity must be less than or equal to 3.

component quantityComparator = LessThan(248);
quantityComparator.in[0] <== 3;
quantityComparator.in[1] <== quantity;
quantityComparator.out === 0;

With quantity ≤ 3 to exploit this issue bid needs to be 252-bit number which is not possible due to constraints in BidTicketHasher(). However, if quantity was ≤ 64 it would be possible. In that case, an attacker could with an extremely large fake bid reveal and valid commit proof cause bidTaxes to become enormous which would prevent processing proper claims and practically lead to locked assets in the ConstructionBay contract.

Remediations to consider:

  • No updates are required as it is not exploitable at the moment.
  • In case there is a need to increase the quantity limit or remove the check, add Num2Bits constraints for the inputs of the amountComparator (minimumAmount, usableDepositAmount).
Q-21

Non-intended bidder behavior could arise if the auction’s flatTaxPercentage is higher than its noShowTaxPercentage

Topic
Incentive Design
Status
Quality Impact
Low

In ConstructionBay.sol, if an auction is setup where its flatTaxPercentage is higher than its noShowTaxPercentage a user would be incentivized to (1) submit a commit where their deposit amount is close to the bid amount, (2) wait for other users to first reveal if they are going to win the auction, (3) choose to not reveal their claim if they see they won’t win in order to: (4) pay the lower fee of flatTaxPercentage than the noShowTaxPercentage.

Remediations to consider if this behavior is not desirable:

  • Require that the flatTaxPercentrage is lower than the noShowTaxPercentage.
I-1

Rewarding depositors who are not really bidding may be ineffective

Topic
Incentive Design
Status
Impact
Informational

In ConstructionBay, Citadel system rewards depositors who are depositing assets when submitting bids with 0 bid and/or 0 quantity. flat-tax is not charged for these users since it is proportional to bid * quantity, which is 0 in this case. However, these users are able to claim depositor rewards proportional to their deposit.

This has been implemented in this way initially in order to obfuscate how much real auction participants are bidding. However, due to the potentially highly predictive behavior of this type of user, their action may not provide any additional benefit to the system, as analyzing and detecting users who submitted 0 bids to the previous auctions may strongly indicate their behavior on the current auction.

In addition, the current implementation based on zero-knowledge proofs already enables individual users to hide their real bids by submitting larger amount and usableAmount when bidding.

Response by Citadel

Wontdo - We’ve discussed with the team and confirmed that the the current behavior is intentional.

I-2

Depositor farmers can lose net funds if there is a token transfer tax on the used IERC20

Topic
Incentive Design
Status
Impact
Informational

It can be non-intuitive to deposit farmers that they can loose money on deposit farming if the auction’s specified ERC20 is one which has a token transfer tax enabled.

Response by Citadel

Addressed - Will include a warning on the construction bay frontend when transfer taxes apply

G-1

Unnecessary memory operations

Topic
Gas Optimization
Status
Gas Savings
Low

Some memory operations in CitadelGovernor::veto() can be eliminated by leveraging a compile-time constant for the relinquishVetoPower() function selector:

contract CitadelGovernor is
  ...
{
  bytes private constant SELECTOR_relinquishVetoPower = abi.encodePacked(this.relinquishVetoPower.selector);

  ...

  function veto(
    ...

    address[] memory us = new address[](1);
    us[0] = address(this);
    uint256[] memory noValue = new uint256[](1);
    noValue[0] = 0;
    bytes[] memory relinquishCall = new bytes[](1);
    relinquishCall[0] = SELECTOR_relinquishVetoPower;

    ...
G-2

Unnecessary function overrides

Topic
Gas Optimization
Status
Gas Savings
Low

Deployment cost for CitadelGovernor.sol can be reduced by eliminating unnecessary function overrides: votingDelay(), votingPeriod(), quorum(), getVotes(), propose(). These purely call super.[sameMethod], and compilation currently succeeds without them.

G-3

MSTORE/MLOAD operations can be eliminated

Topic
Gas Optimization
Status
Gas Savings
Low

In CitadelProxyAdmin::onlyUpgrader():

modifier onlyUpgrader(TransparentUpgradeableProxy proxy) {
  require(
    msg.sender == owner() || msg.sender == _upgraderByProxy[address(proxy)],
    "Sender does not have sufficient permissions"
  );

  _;
}

The upgrader != address(0) check was unnecessary, eliminating the need for upgrader to be stored as a memory variable. Being used only once each, upgrader and owner variables may then be removed without sacrificing code clarity.

G-4

Unnecessary Oracle Consumer

Topic
Gas Optimization
Status
Gas Savings
Low

RandomOracleConsumer is un-utilized within Game.sol and can be removed to reduce deployment cost. This also provides an answer (”no”) to the question on line 461 of 3_deploy_contracts.js:

G-5

Storing multiple variables in memory to save on SLOADs

Topic
Gas Optimization
Status
Gas Savings
Low

In ConstructionBay’s revealBidOnTime function, stateByAuction state variable is used multiple times. Consider to store stateByAcution[auctionId] into memory variable, converting multiple SLOADs into 1 SLOAD.

G-6

Storing token in memory to save on SLOADs

Topic
Gas Optimization
Status
Gas Savings
Low

In ConstructionBay’s deposit function, consider to use memory variable token instead of reading from storage again.

IERC20 token = stateByAuction[auctionId].token;  

uint256 contractTokenDifference = token.balanceOf(address(this));

stateByAuction[auctionId].token.transferFrom(      // use memory variable token here
  msg.sender,
  address(this),
  amount
);

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Citadel team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.