Reach out for an audit or to learn more about Macro
or Message on Telegram

Citadel A-3

Security Audit

June 26th, 2023

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 June 26, 2023 to June 30, 2023.

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
Low 2 - - 2
Code Quality 5 - - 5
Gas Optimization 1 - - 1

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:

Specific changes (PR#19) to add alternate auth system

./src/

Contract SHA256
src/auth/dc/DelegateCashStrategy.sol

32f4e6224314ded43336045e08eea3c0d9a8176aea424870dd1bf5cc3a4b69d3

src/auth/dc/IDelegationRegistry.sol

bd7ff2d2aaaf79e8f911fff6ea3a5ec084665fd5d15c9161a8ca911de5aedb13

src/auth/self/SelfAuthStrategy.sol

e150d5ceb6834347be91f14fcff54b69a04c83671fb9273aa951e66a9abb478f

src/auth/session/ISessionStrategy.sol

48e1c7f34adf29112960897e994dc24961b5eef29a18af9432cdd99254150eab

src/auth/session/IWMATIC.sol

d01c61a7ffe6cae339a4221a707e0efc0be7a77b203940b033946703a8a83acf

src/auth/session/SessionStrategy.sol

9e11e8f5cd3b51891ee776748172d2156cfc2c7788330bbf390abccdc84be34e

src/auth/session/WMATIC.sol

7043188d6cf444ef261e16170d40c6365c37d0b4d09a88d2d124f8bdccc44f2b

src/auth/AuthCompatible.sol

cb3116953fa017687e995bbab23974100b400ad346a767069de9e31ae03ecc89

src/auth/AuthManager.sol

f81b09501d7a9fef53dc7558108bde6f2f60ae435a7e2844c5eeb304d9308221

src/auth/BaseAuthCompatible.sol

c65f16cdca5020ee6783feb538b13dd1b207fdbb2e0fd7bc1a33baba9f1dc362

src/auth/IAuthManager.sol

9bc2416c6db5eba9f91dddf6724d303286890d92461ca3030baa971e472718c4

src/auth/IAuthStrategy.sol

9ae7c413d39e1897e0f93e62bc542ff59f997de33e57acb12901390488f1e771

src/game/access/AccessControlFacet.sol

22117430d653f4d15dfdc0e966b5cc9ab862b83b8030cab34ea82efc893ad61c

src/game/access/AccessControlPrivileged.sol

7021a2d64f2c43a06bc2955f12cf7558a1287509be341bb420725a9d6101828e

src/game/auth/AuthFacet.sol

8e334e6d7bef456081c356c9e7884b762a51ada4db24276e0ed61ae17a6bb61e

src/game/auth/AuthPrivileged.sol

7ad03b17084a2353478aa38d1bc44c3ab820643c0494839d89beee2e5941c3fb

src/game/auth/IAuth.sol

b2d8f780460dc0490f9c6184b2f327dedbe7ccfdfbe63e35b6c9e95d95975c9a

src/game/common/CommonPrivileged.sol

5df8433d239441252caf1d13505a5208376c3b7156f839494ebd976ef00c8f92

src/game/glue/GlueFacet.sol

4274815bcd3fbb1f22774a8cf6ea72646b9e604dc8f4f7e8027f3e52fc0d374a

src/game/upgrades/UpgradesPrivileged.sol

b5bcc49c5b61cd7417a8dcfad295154ea841df51e681c8d6edd07366e6efa53e

src/game/GameInit.sol

5cabdb31b4a7f697f94ee5fb787de79c9544540437d062a9afa2940b3966ee7e

src/game/IGame.sol

11c4be575782cca00d61a3a786e8aaa98f0b1c04b28225a469cbc6fc93f7baf6

Specific changes (PR#20) use separate contracts for the ship token and genesis sale

./src/

Contract SHA256
src/ships/genesis/DutchAuctionSale.sol

c5e2445ecde7cef153d948a94691944379119db6d805df0f7ca2969186ef6703

src/ships/ERC721C.sol

66cc1b9cdcf3d7c020e95173889e4360822faa387d59d0864198b23dd624411f

src/ships/IERC721C.sol

16cca66e03d2f58153b611f68a6b60308fb50c6f11525190e22d8e792389110d

src/ships/IMetadataLib.sol

b0273e48482d9fa5ddb9e13aa18f75706d94c77a4cc0d8abe3a1f684aaec40df

src/ships/IShips.sol

72850e54f02197049b8a2207af1ac588e364ab42c49d4607b3b4e012e88a88d7

src/ships/MetadataLib.sol

0f6f3153311194c0770fa067a071bff6d3d1cd6d7f282c1e4e22d8e0491d7ef7

src/ships/Ships.sol

c23a1170222977d1d0043eda5c2fa0d445b50538bb7c06d0dbfa51a2ed7418e6

src/ships/ShipsMock.sol

9954f161e912b191dc23d364c270e10b30dd903d3e6e442e6ea291a3a0082f64

Specific changes (PR#21) to add setContractURI to ships to reduce the necessity for upgrades

./src/

Contract SHA256
src/ships/Ships.sol

c23a1170222977d1d0043eda5c2fa0d445b50538bb7c06d0dbfa51a2ed7418e6

Specific changes (PR#23) to make existing events more normalized, add some missing events that may be useful

./src/

Contract SHA256
src/auth/AuthManager.sol

f81b09501d7a9fef53dc7558108bde6f2f60ae435a7e2844c5eeb304d9308221

src/auth/IAuthManager.sol

9bc2416c6db5eba9f91dddf6724d303286890d92461ca3030baa971e472718c4

src/game/belts/BeltsPrivileged.sol

f90d6488bd000e3f0d17f7e1089cef5531f2f825115b61468015d81c28039dc0

src/game/belts/IBelts.sol

6c0f227accb574a70c2963851c68fc285fc784d4a24495fbbcd0245c582853db

src/game/common/CommonPrivileged.sol

5df8433d239441252caf1d13505a5208376c3b7156f839494ebd976ef00c8f92

src/game/common/ICommon.sol

5d08db2cab4adcd0f3d601d62818933dbdca652f4cd3fcfcd6823bfc0a25f27f

src/game/glue/GlueFacet.sol

4274815bcd3fbb1f22774a8cf6ea72646b9e604dc8f4f7e8027f3e52fc0d374a

src/game/glue/IGlue.sol

9bc8d260f8b10c6c47cd6668a7ffcb166f7618aa8f772a9e3a8302ebaf721e35

src/game/outposts/IOutposts.sol

e5c285452dfad49c9939c54288629a67340aebaf8bf75faee03c5feada200762

src/game/rewards/IRewards.sol

cf3d5d96b522328a4c62ec9f837d33ad9a5c1df474f33b865b433c1143a6ff2f

src/game/rewards/RewardsPrivileged.sol

22827f43348934dba2e01b069dcabe05c51013650e5520a2cbb43ba59a9c06f0

src/game/travel/ITravel.sol

060b7cf9f9f9c612f7f74321729deaf2ea367ed2ef67ef7af481eebe6ce243d9

src/game/travel/TravelPrivileged.sol

b129b13c6ef7e9d56a7e0a1218258e530406a09100ec65b6b00fe7d99fbd01ca

src/game/upgrades/IUpgrades.sol

ddaf819021cd75f6ce44219e1d17efcc4c4ab3081d523bcd96716ccd03273ea9

src/game/upgrades/UpgradesPrivileged.sol

b5bcc49c5b61cd7417a8dcfad295154ea841df51e681c8d6edd07366e6efa53e

src/governance/CitadelGovernor.sol

5fd212965410624879041f2710c2e9c43ddf3a430af76269e8d22f60ec70511a

src/ships/genesis/DutchAuctionSale.sol

c5e2445ecde7cef153d948a94691944379119db6d805df0f7ca2969186ef6703

Specific changes (PR#24) to fix audit report findings

./src/

Contract SHA256
src/base/util/IStructs.sol

9645c261f7b5fb67e4d322c907e5d24305c5cf26e8c23eeed0ea4522a987afce

src/game/belts/BeltsPrivileged.sol

f90d6488bd000e3f0d17f7e1089cef5531f2f825115b61468015d81c28039dc0

src/game/rewards/RewardsPrivileged.sol

22827f43348934dba2e01b069dcabe05c51013650e5520a2cbb43ba59a9c06f0

src/game/travel/ITravel.sol

060b7cf9f9f9c612f7f74321729deaf2ea367ed2ef67ef7af481eebe6ce243d9

src/game/travel/TravelFacet.sol

f4b27563da4af0be4f6d255efaaaaa56874919f15e9ca75165e2cfb41500af41

src/game/travel/TravelPrivileged.sol

b129b13c6ef7e9d56a7e0a1218258e530406a09100ec65b6b00fe7d99fbd01ca

src/game/upgrades/UpgradesPrivileged.sol

b5bcc49c5b61cd7417a8dcfad295154ea841df51e681c8d6edd07366e6efa53e

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

L-1

SessionEnded emitted with incorrect arguments

Topic
Events
Status
Impact
Low
Likelihood
Low

The SessionStrategy:endSession() emits SessionEnded event with delegate and vault arguments in an incorrect order.

emit SessionEnded(delegate, _session.account, remaining);

In emission, a delegate is 1st and the vault is the 2nd argument. However, in the event declaration vault is 1st and delegate is 2nd argument.

event SessionEnded(address vault, address delegate, uint256 fundingReturned);

Remediations to Consider:

  • Update event emission to be consistent with the event declaration.
L-2

endSession() won’t emit SessionEnded event if the remaining allowance is zero

Topic
Events
Status
Impact
Medium
Likelihood
Low

In SessionStrategy.sol's, all endSession() calls with zero allowance won’t emit the corresponding SessionEnded event.

**if** (remaining == 0) **return**;

...

emit SessionEnded(delegate, _session.account, remaining);

remaining could be zero if:

  • The delegate spends all the allowed wrapped MATIC.
  • The session started without any remaining value.
  • The delegate did not have any registered session.

Remediations to Consider:

  • Emitting the event even if there is no remaining allowance in the case when the id of the session is not zero.
Q-1

Event emitted even if no calls were executed

Topic
Code Quality
Status
Quality Impact
Medium

If no targets/calldatas parameters are sent into the sendCalls function call, the transaction will still succeed and emit CallsSent event, even if no calls were executed. Consider checking whether at least one function input (targets.length and calldatas.length) is provided.

Q-2

Missing indexed parameter in events

Topic
Code Quality
Status
Quality Impact
Low
  • BeltPublished event should have beltId indexed.
  • CallsSent event should have strategy, vault, delegate indexed.
  • SessionStarted and SessionEnded events should have vault and delegate indexed.
Q-3

Improve Natspec documentation

Topic
Code Quality
Status
Quality Impact
Low
  • Natspec comments missing for strategy param in CallsSent event declaration in IAuthManager

  • Missing Natspec comments on state variables in the interface ISessionStrategy

    • In ISessionStrategy.sol the following structs do not have any natspec comments. Consider adding natspec comments so it’s clear what these structs are meant to track.

      struct TargetPair {
        address target;
        bytes4 selector;
      }
      
      struct Session {
        address account;
        uint32 endTime;
      }
      
  • Typo in the IAuthStrategy.isAllowed() Natspec comment

    • There’s a typo in the @notice tag, where vault is typed as value
  • Missing Natspec for IERC721C:addressInfoOf() function.

  • Missing Natspec comments for safeMintBatch() in the interface IShips.

Q-4

startSession() executes unnecessary external calls with zero values

Topic
Code Quality
Status
Quality Impact
Medium

In SessionStrategy, startSession() will approve and deposit the difference between the msg.value and the input parameter sessionGasFunding. However, if only the sessionGasFunding value was provided in the call, the logic will still execute the external calls to the wrapped MATIC contract will zero values. To avoid unnecessary external calls, consider only calling the wrapped external contract if msg.value - sessionGasFunding > 0.

Q-5

Missing initialization of ReentrancyGuardUpgradeable contract in DutchAuctionSale

Topic
Code Quality
Status
Quality Impact
Low

In DutchAuctionSale:initialization() function, there is no call to __ReentrancyGuard_init(). While this does not affect system operation, when inheriting from upgradable contracts such asReentrancyGuardUpgradeable, consider calling appropriate initializers to set their corresponding state.

G-1

Consider caching the result of checkDelegateForContract() call

Topic
Gas Optimisation
Status
Gas Savings
Medium

In DelegateCashRegistry, isAllowed() has following implementation:

function isAllowed(
    address delegate,
    address vault,
    address[] memory targets,
    bytes[] memory
  ) external view returns (bool) {
    for (uint256 i = 0; i < targets.length; i++) {
      if (
        !delegationRegistry.checkDelegateForContract(
          delegate,
          vault,
          targets[i]
        )
      ) return false;
    }

    return true;
  }

Since the result of the call doesn’t depend on the calldata but only on the address of the target contract, due to batching use case, it is probable that many targets will be the same contract address. In that case, an external call for each is unnecessary and can be avoided with function-level caching.

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.