Security Audit
March 2, 2023
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
c59d36a7f8d507fef12a6eeb1cf32b7633bb50d9
c59d36a7f8d507fef12a6eeb1cf32b7633bb50d9
d35c52a86214b1f89b0e65db3237e2148100d6d9
Specifically, we audited the following contracts as part of the core-contracts audit
Contract | SHA256 |
---|---|
utils/core-contracts/contracts/token/ERC20.sol |
|
utils/core-contracts/contracts/token/ERC20Helper.sol |
|
utils/core-contracts/contracts/token/ERC20Storage.sol |
|
utils/core-contracts/contracts/token/ERC721.sol |
|
utils/core-contracts/contracts/token/ERC721Enumerable.sol |
|
utils/core-contracts/contracts/token/ERC721EnumerableStorage.sol |
|
utils/core-contracts/contracts/token/ERC721Owned.sol |
|
utils/core-contracts/contracts/token/ERC721Storage.sol |
|
We audited following files as part of the hardhat-router audit
Contract | SHA256 |
---|---|
utils/hardhat-router/src/extensions/timers.ts |
|
utils/hardhat-router/src/index.ts |
|
utils/hardhat-router/src/internal/contract-helper.ts |
|
utils/hardhat-router/src/internal/deploy-contract.ts |
|
utils/hardhat-router/src/internal/errors.ts |
|
utils/hardhat-router/src/internal/fourbytes.ts |
|
utils/hardhat-router/src/internal/quiet-compile.ts |
|
utils/hardhat-router/src/internal/render-router.ts |
|
utils/hardhat-router/src/internal/render-template.ts |
|
utils/hardhat-router/src/internal/render-testable-storage.ts |
|
utils/hardhat-router/src/internal/router-function-filter.ts |
|
utils/hardhat-router/src/internal/router-helper.ts |
|
utils/hardhat-router/src/internal/timed.ts |
|
utils/hardhat-router/src/internal/validate-interfaces.ts |
|
utils/hardhat-router/src/subtasks/generate-router.ts |
|
utils/hardhat-router/src/subtasks/generate-testable-storage.ts |
|
utils/hardhat-router/src/subtasks/index.ts |
|
utils/hardhat-router/src/subtasks/validate-interfaces.ts |
|
utils/hardhat-router/src/subtasks/validate-selectors.ts |
|
utils/hardhat-router/src/task-names.ts |
|
utils/hardhat-router/src/tasks/deploy.ts |
|
utils/hardhat-router/src/tasks/generate-testable.ts |
|
utils/hardhat-router/src/tasks/index.ts |
|
utils/hardhat-router/src/tasks/upload-selectors.ts |
|
utils/hardhat-router/src/types.ts |
|
We also reviewed following files as part of the hardhat-storage audit
Contract | SHA256 |
---|---|
utils/hardhat-storage/src/index.ts |
|
utils/hardhat-storage/src/internal/dump.ts |
|
utils/hardhat-storage/src/internal/error.ts |
|
utils/hardhat-storage/src/internal/iterators.ts |
|
utils/hardhat-storage/src/internal/misc.ts |
|
utils/hardhat-storage/src/internal/render.ts |
|
utils/hardhat-storage/src/internal/validate-namespace.ts |
|
utils/hardhat-storage/src/internal/validate-variables.ts |
|
utils/hardhat-storage/src/internal/validate.ts |
|
utils/hardhat-storage/src/subtasks/get-source-units.ts |
|
utils/hardhat-storage/src/subtasks/parse-contents.ts |
|
utils/hardhat-storage/src/subtasks/parse-dump.ts |
|
utils/hardhat-storage/src/task-names.ts |
|
utils/hardhat-storage/src/tasks/verify.ts |
|
Click on an issue to jump to it, or scroll down to see them all.
receive()
function is not useable
approval()
VALIDATE_SELECTORS
subtask does not check for duplicate selectors in the Proxy contract
fallback()
function
supportsInterface()
deploy
task only sends modules
folder for storage collision check
core-contracts
storage hashing
abi.encodeCall
is invalid for some compiler versions
core-contracts
folder rehashes some static storage slots
We quantify issues in three parts:
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. |
receive()
function is not useable
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:
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.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
0
ownerOf
0
tokenURI
“”
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
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
approval()
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:
approve()
is called by Bob to allow Alice to spend 200 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
increaseAllowance
and decreaseAllowance
, similar to OpenZeppelin, such that the allowance is increased and decreased respectively.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
initialValue
parameters of functionNode
fetched from iterateSlotAssignments()
after being sure .slots
are assigned to literal values.VALIDATE_SELECTORS
subtask does not check for duplicate selectors in the Proxy contract
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
validate-selectors
subtask.fallback()
function
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:
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.We’re fine with not allowing modules to have a fallback function.
supportsInterface()
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
supportsInterface
according to EIP-721.deploy
task only sends modules
folder for storage collision check
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
core-contracts
storage hashing
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.
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.
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.
abi.encodeCall
is invalid for some compiler versions
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.
core-contracts
folder rehashes some static storage slots
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:
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.