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

Synthetix 1

Security Audit

March 2, 2023

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

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

The purpose of this audit is to review the source code of certain Synthetix 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
High 5 - - 5
Medium 4 - 1 3
Code Quality 3 1 - 2
Gas Optimization 1 - - 1

Synthetix 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 core-contracts audit

Contract SHA256
utils/core-contracts/contracts/token/ERC20.sol

3fbc321cf8fc6fd657013c57b3e4a9bc4eb57903d63f9425b3519b1ef80c021f

utils/core-contracts/contracts/token/ERC20Helper.sol

ee4d5889b54f47d36c40e6497a8d1fcd7bc4fdb9c1382ef04007a54a21ae92b1

utils/core-contracts/contracts/token/ERC20Storage.sol

92bdd5b7dae758caa8e75ff7dd91c1a4b5e48dd70b056903c8c7eca93cece226

utils/core-contracts/contracts/token/ERC721.sol

0a92f19fbad3c98f52112486f42f4d9e1b60c42412e236891b1b1008d5f6ae47

utils/core-contracts/contracts/token/ERC721Enumerable.sol

b144a09b3e68cfa9df8da4054fcb6cb19b36501582134af542a45aab1530afbf

utils/core-contracts/contracts/token/ERC721EnumerableStorage.sol

d480d61f2c463c83e6cbcd88751a8ea9f3c58a5d36573dc24be14d57b7a187ba

utils/core-contracts/contracts/token/ERC721Owned.sol

fe0f0f12437fb96a9c72e79756a0a0fc03aa0d9fc02568db4d19750b4b0dacf9

utils/core-contracts/contracts/token/ERC721Storage.sol

65346abe209fca65043a98ac02a3c689232d0615ed0def9c12b40cba4602a5cc

We audited following files as part of the hardhat-router audit

Contract SHA256
utils/hardhat-router/src/extensions/timers.ts

912c6682eaadfe5e8f43957217578c4c64556ad6c738c0c8691069b37d9e513e

utils/hardhat-router/src/index.ts

d1c7460c3a397a9800d12f48e6694f1903b41488b6b8eca5791371602c006ba9

utils/hardhat-router/src/internal/contract-helper.ts

836b0c3dd16a0bbc2bfd897b8299ad88ed0d9b2af5c68b7699ce940ca32c082b

utils/hardhat-router/src/internal/deploy-contract.ts

c31d7a7e5a67085ede323d7198f010c7a9cbd09e3780e0c56bb33fe9423b80cb

utils/hardhat-router/src/internal/errors.ts

fbe554992b38fb59a52965172549ec7908d66e43aec97136f9bb2b6774e512df

utils/hardhat-router/src/internal/fourbytes.ts

36b1b849551160c7a4f65baaf9ca9a2225cc978500a67de15136a3d135fb01da

utils/hardhat-router/src/internal/quiet-compile.ts

635cfc8530a5c99d35a5faf6b8c26d45b19b73e139e1f75dd3599fc2fd1d34a1

utils/hardhat-router/src/internal/render-router.ts

70fe04a4fe742734b19cfc01bbf87d361e857a5d7591753051615258cf6948e8

utils/hardhat-router/src/internal/render-template.ts

5bd60064fb98c967c709915ee1bbec1d86c53d58bd361f956b713b59c5b0b2e1

utils/hardhat-router/src/internal/render-testable-storage.ts

02fbd8cd188fd4e1eac53b711b4b31e8d5ac3f12fe8410e1f22999c4f8115f7a

utils/hardhat-router/src/internal/router-function-filter.ts

a266c468d8ec269d5c07a60cf33c2590c747e8604053e0e4c8ee26a9542af309

utils/hardhat-router/src/internal/router-helper.ts

d58d0384d0d3f82a63a54ca12801e46997889aa0058bca6a3f32cac73d6c596e

utils/hardhat-router/src/internal/timed.ts

b0cd60033efcfac3d248fd375be4b3d94ba205ffa9f8a9e8f631c2e27bc85b98

utils/hardhat-router/src/internal/validate-interfaces.ts

d4f8b815a235422b4c921edf9c7d9f0007c5dec98dd69ee7a01e23a275faef28

utils/hardhat-router/src/subtasks/generate-router.ts

4640d77649e0a252c4f45431a69db94baedf084465565bf074c00526b723af50

utils/hardhat-router/src/subtasks/generate-testable-storage.ts

fd85b3e628c0bab9be40e74d632c721184414302ebae8ae5928bd36db90ac801

utils/hardhat-router/src/subtasks/index.ts

d0ebe9ca4c195e78574a736fed76e66ee0bab67487743a139ce82cf6ea2100e5

utils/hardhat-router/src/subtasks/validate-interfaces.ts

0b6a23fd7f043f1133fd42e23fa3ddf927d358ad9930f0bafad0917685489f1a

utils/hardhat-router/src/subtasks/validate-selectors.ts

6bb32480394be1cc35178f06bbc144c016caf5affe8e2251b2d90c5659d14a69

utils/hardhat-router/src/task-names.ts

8891baf24b09432da5e406a8e72c6b3535f6031a4314d838378cf7b9f20bec5e

utils/hardhat-router/src/tasks/deploy.ts

8327273cbd950968f84702aa78e7ee6db9a29ab0f241cd95fa7b7968010d1832

utils/hardhat-router/src/tasks/generate-testable.ts

701772f552cf13d270c962e50abea28afb499466a96b5e780c693eeb75fa455b

utils/hardhat-router/src/tasks/index.ts

39d566a6e98735b80d349b3b84bfbe70918ef9a5d8b172a8ca4209097166e935

utils/hardhat-router/src/tasks/upload-selectors.ts

ca9746ebb004f76d9d62e7002024051da94d101bffe519a83ca6617e92028e97

utils/hardhat-router/src/types.ts

5eb134c62dca0a9c191f26e02e7d66233059007e766b881e0713e59a48f629ae

We also reviewed following files as part of the hardhat-storage audit

Contract SHA256
utils/hardhat-storage/src/index.ts

95c1e92a5a0adeba3b1d2facd47cd89f609682dc3a51ed46e79ac48cf99b8fb9

utils/hardhat-storage/src/internal/dump.ts

8e5e2003dcc1bb13ab243fdcc1a776efc163865bf2522936861663ebc717524a

utils/hardhat-storage/src/internal/error.ts

e412d6e3f811d745b1ffb03af8a9d627322348738a04836ffec741ccd3fd2238

utils/hardhat-storage/src/internal/iterators.ts

2585cd0e4caf3e521bd6d5f51a3b9f8e2df012a6f616a42b7941e70aa7497e67

utils/hardhat-storage/src/internal/misc.ts

30b6cef2beb24d12d802663fca1595ea54c023abbea1184e9591b2f225ca773f

utils/hardhat-storage/src/internal/render.ts

b0b5ed7fe2a529535d7d60839edcad4f8a02da32752f22a032e014cffcf4843d

utils/hardhat-storage/src/internal/validate-namespace.ts

1545c0fbe76ce0b8c0a7293d785b2636a8b8cc69b7bd513dbf1cf78ed08a966e

utils/hardhat-storage/src/internal/validate-variables.ts

374357e2880a062a5e672566e4efe5716324e5b7d4f3235ddf13d088df2c135c

utils/hardhat-storage/src/internal/validate.ts

ba3556335eb37b282d499cae9e1e7d13b834abc4f92c9004eededd5db6f050bd

utils/hardhat-storage/src/subtasks/get-source-units.ts

27399f7250c4318270b45ad21f236de676e3d0ac2c4c77a2e44a920b158d41bb

utils/hardhat-storage/src/subtasks/parse-contents.ts

0e4ba9573bf6dedf624eb1c96354ae2e474b85e4e76af8e71b20d7d1f677ae98

utils/hardhat-storage/src/subtasks/parse-dump.ts

7202e5f1da39067fa5fe9544c2d74498108f3e66c49b53b28458308002879b1b

utils/hardhat-storage/src/task-names.ts

c34af95023b238869acef6be33998a145d7496a784ffad7e4d876807532de603

utils/hardhat-storage/src/tasks/verify.ts

b98cd1eddab401ca31d225505bb9727f7725d37e976b036d0674e3af9d965ebf

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

H-1

Router’s receive() function is not useable

Topic
Use Cases
Status
Impact
High
Likelihood
Medium

In the generated Router.sol, the receive() function attempts to forward calls to the _forward() function:

receive() external payable {
    _forward();
}

This is problematic because the _forward() function itself does not have the capability to forward to underlying module’s receive() functions. This is because the picked-up function selectors from modules do not include the standard receive() function. This causes the invocation of the receive() function to always revert, breaking the idea that the contracts built with Router.sol have the full capabilities of normal contracts.

Currently, the hardhat-router plugin implementation filters out the receive() function by using the getSelectors() function defined in core-utils/src/utils/ethers/contracts.ts. This function filters by fragments with the type “function" while the recieve() ABI entry is of type “receive”.

Remediations to consider:

  • The receive() function can be treated like the other module contract’s publicly accessible functions. It appears in a contract’s ABI, can be assured to be singular, and can be forwarded to in the same way. This would enable behavior uniformity with vanilla EVM contracts.
H-2

ERC721 functions do not comply with EIP-721 standard

Topic
Coding Standards
Status
Impact
High
Likelihood
Low

In ERC721 from utils/core-contracts/contracts/token/ERC721.sol, all the functions should adhere to the EIP-721 spec:

NFTs assigned to zero address are considered invalid, and queries about them do throw.

In Synthetix implementation in, these functions do not throw but return the following values:

  • balanceOf
    • Returns 0
  • ownerOf
    • Returns 0
  • tokenURI
    • Returns “”

Since the goal of specifications is to allow interoperability, external projects that rely on Synthetix’s ERC721 implementation will receive unexpected results. This can cause problems for the projects building on Synthetix’s core-contracts.

Remediations to Consider

  • Adhering to the EIP-721 standard and revert when queried IDs are invalid i.e. non-existent.
H-3

ERC721Enumerable functions do not comply with EIP-721 standard

Topic
Coding Standards
Status
Impact
High
Likelihood
Low

In ERC721Enumerable from utils/core-contracts/contracts/token/ERC721Enumerable.sol, some of the functions do not adhere to the EIP-721 spec.

The function tokenOfOwnerByIndex accepts an owner and index of the owner’s token list. Currently, it returns 0 if the index to be queried is greater than or equal to the owner’s balance. Instead, it should revert. Also, it should throw when checking the token balance of an non-existent owner (this can be addressed in issue H-2).

Another function, tokenByIndex also does not follow the spec. Currently, it returns 0 if the index is greater than or equal to the total supply, while instead it should also revert.

Since the goal of specifications is to allow interoperability, external projects that rely on Synthetix’s ERC721Enumerable implementation will receive unexpected results. This can cause problems for the projects building on Synthetix’s core-contracts.

Remediations to Consider

  • Adhering to the EIP-721 standard.
H-4

ERC20 implementation does not provide a safe alternative to unsafe approval()

Topic
Coding Standards
Status
Impact
High
Likelihood
Low

According to EIP-20, the approve() method of ERC20 in utils/core-contracts/contracts/token/ERC20.sol has an attack vector related to directly setting the approval amount. This opens up a user to a front-running attack when used multiple times on the same spender.

To illustrate, consider:

  1. approve() is called by Bob to allow Alice to spend 200 tokens.
  2. Before Alice spends the tokens, approve() is called again by Bob to allow Alice to spend 100 tokens instead of 200 tokens.

After the 2nd approval, Bob expects that Alice is able to only spend a total of 100 tokens. However, Alice is able to front-run the second approve() by transferring 200 tokens that was approved in the first approve(). Afterwards, Alice is able to spend another 100 tokens. The result is Alice is able to transfer 300 tokens instead of the 100 or 200 that Bob expected.

An alternative to using approve directly is to call a function that increases the approval amount instead of directly changing it. This is currently done in some implementations such as OpenZeppelin.

Remediations to Consider

  • Implement increaseAllowance and decreaseAllowance, similar to OpenZeppelin, such that the allowance is increased and decreased respectively.
H-5

Slots are compared by variable names which may fail to prevent storage collisions

Topic
Storage Collision
Status
Impact
High
Likelihood
Medium

validateSlotNamespaceCollisions() in validate-namespace.ts checks the contracts for yul code that assigns storage slots and if the same slot is assigned in two different places it throws an error and blocks the deployment. However, this check is based on the variable name that is used to hold the bytes that correspond to the slot number and not the value of the variable. This means if two variables with different names hold the same value they will pass the collision check, and if two different slot variables share a name but not value, they will wrongly throw an error.

Remediations to Consider

  • Instead of comparing storage slot variable names, the tool should ensure there are no duplicate storage slot values assigned twice. This can be achieved by comparing the initialValue parameters of functionNode fetched from iterateSlotAssignments() after being sure .slots are assigned to literal values.
M-1

VALIDATE_SELECTORS subtask does not check for duplicate selectors in the Proxy contract

Topic
Selector Collision
Impact
High
Likelihood
Low

The hardhat-router-validate-selectors subtask is used to check if there are any duplicate function selectors to prevent selector clashes between modules. Checking only the modules folder may fail preventing all selector clashes, as there is nothing that prevents users from having functions in their Proxy contracts.

Which Proxy contract to use is determined by the dev by passing the path of the Proxy to the tool (with the default of 'contracts/Proxy.sol:Proxy’). The dev, maliciously or by chance, may include a function in the Proxy contract that clashes with one of the functions in the modules, or, a newly added module may introduce a clash with an existing Proxy function.

As discussed with the Synthetix team, the Router is intended to be 100% explicit about all system functionality. Having such a clash will cause unexpected results for users who think the system behaves as it is represented in the Router. The issue might be exploitable in permissionless protocols where anyone can introduce a new module.

Remediations to Consider

  • Include Proxy contract’s functions to the list for function selector clash check in validate-selectors subtask.
  • Or, validate there are not any functions in the Proxy contract.

M-2

Router’s module contracts cannot specify a fallback() function

Topic
Use Cases
Status
Wont Do
Impact
Medium
Likelihood
Medium

Currently the generated Router.sol has no functionality to allow for its underlying module contracts to specify a fallback() function for their own use cases. This creates a difference between the capabilities of Router-based and vanilla contract implementations. The cause of this is similar to that of [H-1], as the fallback() function is of type “fallback" and not type “function" in the ABI and is not under consideration of the Router to be routed to.

Remediations to consider:

  • The fallback() function can be treated like the other module contract’s publicly accessible functions. It appears in a contract’s ABI, can be assured to be singular, and can be forwarded to in the same way. This would enable behavior uniformity with vanilla EVM contracts.
Response by Synthetix

We’re fine with not allowing modules to have a fallback function.

M-3

ERC721Enumerable does not extend supportsInterface()

Topic
Spec
Status
Impact
Medium
Likelihood
High

In ERC721Enumerable, the supportsInterface() function has not been properly extended. Instead it uses the ERC721 base implementation. According to EIP-721, in the interface ERC721Enumerable section, the comment states :

Note: the ERC-165 identifier for this interface is 0x780e9d63.

Currently, if a user wants to check if the contract supports ERC721Enumerable and calls ERC721Enumerable.supportsInterface(0x780e9d63), they would receive false instead of the spec-required true.

Remediations to Consider

  • Implement the supportsInterface according to EIP-721.
M-4

deploy task only sends modules folder for storage collision check

Topic
Storage Collision
Status
Impact
High
Likelihood
Low

Similar to issue M-1, the TASK_STORAGE_VERIFY is run with only the modules folder. This will cause missing possible storage collisions in the Proxy contract.

await hre.run(TASK_STORAGE_VERIFY, { contracts: modules });

Remediations to Consider

  • Include Proxy contract in the list of to-be-verified contracts in the storage verify task.
Q-1

Inconsistent use of subtraction in core-contracts storage hashing

Topic
Quality
Status
Acknowledged
Quality Impact
Low

In Synthetix.io’s v3 repo, the core-contracts folder and other folders sometimes suggests adherence to the pattern of subtracting one from hashes when creating storage slots as in ProxyStorage.sol:

function _proxyStore() internal pure returns (ProxyStore storage store) {
    assembly {
        // bytes32(uint(keccak256("io.synthetix.v3.proxy")) - 1)
        store.slot := 0x32402780481dd8149e50baad867f01da72e2f7d02639a6fe378dbd80b6bb446e
    }
}

This pattern is not followed in all files. For uniformity of the code base, consider choosing whether to subtract 1 or not and use the same pattern everywhere. The subtraction is done typically to ensure that the pre-image of the used hash is unknown. This is not necessarily needed for this use case.

Response by Synthetix

Good point about consistency and that we really don’t need to subtract in this use case. We decided to go without subtraction, and has been implemented in a previous PR that moves all hashes that don’t move into constant variables.

Q-2

Use of custom errors in generated router file is invalid for some compiler versions

Topic
Quality
Status
Quality Impact
Low

In all contracts, pragma solidity ^0.8.0; is used while the custom errors feature was released in version 0.8.4. This will cause any version before 0.8.04 to fail to compile. Consider changing the pragma to a higher version.

Q-3

Use of abi.encodeCall is invalid for some compiler versions

Topic
Quality
Status
Quality Impact
Low

In the UUPSImplementation, abi.encodeCall is used to encode the function calldata. This built-in function was released in version 0.8.11.

In all contracts, pragma solidity ^0.8.0; is used. This will allow any version above 0.8.0 to attempt to compile, but will fail if the compiler version is below 0.8.11. Consider changing the pragma to a higher version.

G-1

core-contracts folder rehashes some static storage slots

Topic
Gas
Status
Gas Savings
Low

In Synthetix.io’s v3 repo, the core-contracts folder rehashes static storage slots in some places. For example, this is the of the OwnableStorage.sol’s storage implementation:

The rehashing is unnecessary and can be replaced with a static storage slot as is done in the repo’s sample-projects folder:

function load() internal pure returns (Data storage store) {
    bytes32 s = keccak256(abi.encode("Ownable"));
    assembly {
        store.slot := s
    }
}
function load() internal pure returns (Data storage store) {
    assembly {
        // bytes32 s = keccak256(abi.encode("Ownable"));
        store.slot := 0xe1550b5a17836cfadda6044cd412df004a72cf007361a046298ac83a7992948c
    }
}

Remediations to consider:

  • Using hardcoded hash outputs instead of rehashing.

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 Synthetix 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.