Security Audit
February 24th, 2023
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for The Graph's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from February 13 to February 14, 2023. Review of fixes was conducted on February 20, 2023.
The purpose of this audit is to review the source code of certain The Graph 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 | 3 | - | - | 3 |
Medium | 1 | - | - | 1 |
Low | 3 | - | 1 | 2 |
Code Quality | 4 | - | - | 4 |
Informational | 1 | - | - | - |
Gas Optimization | 4 | - | - | 4 |
The Graph was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
Entities
Trust Model
General Assumptions
The following source code was reviewed during the audit:
81a44aede886fc972b2f2422038a804f00789966
71a8bff74ef30f1e530ae9c922dcfc5e7a2c7983
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
contracts/Subscriptions.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
subscribe()
not callable for other user after their first subscription expires
extendSubscription()
front-run vulnerability
fulfil()
reverts or loses funds when user has an active subscription
subscribe()
with incorrect values can lock funds in contract
subscribe()
griefing attack
locked()
may produce inaccurate result due to cast truncation
Init
event
require()
collect()
unsubscribe()
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. |
subscribe()
not callable for other user after their first subscription expires
Topic | Use Cases | |
Status | Fixed | |
Impact | Spec Breaking |
|
|
||
Likelihood | N/A |
The subscribe()
function mistakenly compares the user
subscription end date against block.number
instead of block.timestamp
. This prevents a user X from calling subscribe()
for a user Y if user Y has an expired subscription.
Consider updating block.number
to block.timestamp
.
extendSubscription()
front-run vulnerability
Topic | Front-Running | |
Status | Fixed | |
Impact | Critical |
Funds are stolen. |
Funds are stolen. |
||
Likelihood | Medium |
Increases linearly with over-approval amounts. |
Increases linearly with over-approval amounts. |
In extendSubscription()
, the user’s current subscription is used to calculate how many additional tokens are required to extend the subscription up to the specified end
.
However, a user may be able to get the extender to pay more than they intend. For example:
200
to 500
with rate 7
. Current time is 230
.extendSubscription()
with user X’s address and an end time of 800
.unsubscribe()
, also getting a refundsubscribe()
with start
as now, with end
as a small amount after (such as 315
), and with rate
as high as they can afford.250
seconds of time, is now paying for 435
seconds, and at a much higher rate than intended.This issue affects extenders linearly with approval size.
Consider updating extendSubscription
’s parameters to include a intendedRate
so the extender’s paid rate cannot be front-run.
Also consider replacing end
with secondsToExtend
so extenders cannot pay for more time than they intend.
fulfil()
reverts or loses funds when user has an active subscription
Topic | Edge Case | |
Status | Fixed | |
Impact | High |
Common case: Use case is blocked. Worst case: User loses funds from their current subscription, by malicious intent or by grief. |
Common case: Use case is blocked. Worst case: User loses funds from their current subscription, by malicious intent or by grief. |
||
Likelihood | Medium |
User must have an active subscription, but can be griefed into having one. Third party will likely not have a subscription, and can be griefed into having one. |
User must have an active subscription, but can be griefed into having one. Third party will likely not have a subscription, and can be griefed into having one. |
A use case supported by the Subscriptions contract is allowing a third party to call fulfil()
on a user’s pending subscription, converting the pending into an actual subscription and paying for it.
If the targeted user already has an existing subscription, unsubscribe()
is called to refund the user before converting the pending subscription.
However, the unsubscribe()
function only refunds msg.sender
, which is different than the user in this use case. This causes one of the following issues:
msg.sender
has an active subscription, the msg.sender
’s subscription is unsubscribed, and the target user’s subscription is overwritten, losing access to those funds.msg.sender
does not have an active subscription, the call reverts with no active subscription
, and the use case is blocked from functioning.Note that the subscription status of either party can be freely switched from no-subscription to active-subscription by way of subscribe()
, allowing a griefer to set up these scenarios at low cost.
Consider implementing an internal function _unsubscribe(address)
which accepts an address, and calling it from unsubscribe()
with msg.sender
, and from _subscribe()
with user
.
subscribe()
with incorrect values can lock funds in contract
Topic | Input Ranges | |
Status | Fixed | |
Impact | High |
|
|
||
Likelihood | Low |
User would need to call |
User would need to call |
The subscribe()
function allows start
and end
values to have a maximum value of type(uint64).max
, but the start
value can be truncated during cast to int64
on lines 361-362 of unlocked()
:
uint256 len = uint256(
SignedMath.max(
0,
**int256(int64(_subEnd)) -
int256(Math.max(block.timestamp, _subStart))**
)
);
As a result, unsubscribe()
can mis-calculate the refund and leave a portion of funds locked in the contract.
Consider restricting subscription start
and end
to a maximum value of type(int64).max
.
subscribe()
griefing attack
Topic | Open Access | |
Status | Fixed | |
Impact | Medium |
Blocks an uncommon use case. |
Blocks an uncommon use case. |
||
Likelihood | Low |
No one benefits from the attack. |
No one benefits from the attack. |
When user X calls subscribe()
to create a subscription for user Y, a malicious user Z can front-run the transaction by calling subscribe()
themselves with a long end
and a minimal rate
.
This can disrupt the [assumingly less-common] use case of a machine user granting a user a subscription upon some event.
Consider one or many of:
Topic | Ecosystem | |
Status | Wont Do | |
Impact | Medium |
Blocks certain tokens from being used as currency. |
Blocks certain tokens from being used as currency. |
||
Likelihood | Low |
The number of tokens that fall under this category may be limited. |
The number of tokens that fall under this category may be limited. |
Not all ERC-20 tokens return a boolean for their transfer functions. These tokens are not compatible with the Subscriptions contract as currently written, since it requires that they return true
.
Consider using Open Zeppelin’s SafeERC20 helper function if support for these tokens is desired.
Won’t fix. Only applies to nonconforming ERC-20 implementations that don’t return boolean values from
tranfer()
. We don’t intend to support these implementations.
Topic | Re-entrancy | |
Status | Addressed | |
Impact | Low |
Locally, exploiter only loses money. Might create issues for future integrations, though unlikely. |
Locally, exploiter only loses money. Might create issues for future integrations, though unlikely. |
||
Likelihood | Low |
Only applicable if a future integration somehow introduces a vulnerability. |
Only applicable if a future integration somehow introduces a vulnerability. |
The _subscribe
has a minor reentrancy vulnerability:
_unsubscribe
is calledsubscriptions[user]
and the new epoch states get updated.However, this causes the attacker to lose money, as their old subscription data is overwritten with the new, with no chance to retrieve the funds for the old.
This shouldn't cause any problems. However, if extra safety is desired, consider adding OZ's nonReentrant
modifier to subscribe
, unsubscribe
, and fulfil
.
We've added a comment explaining the potential vulnerability.
locked()
may produce inaccurate result due to cast truncation
Topic | Data Views | |
Status | Fixed | |
Quality Impact | High |
Similar to M-2, locked()
also may cause cast truncation and yield incorrect values. See line line 332:
uint256 len = uint256(
SignedMath.max(
0,
**int256(Math.min(block.timestamp, _subEnd)) - int64(_subStart)**
)
);
This function is not utilized internally so no vulnerability is created within this contract.
Consider restricting subscription start
and end
to a maximum value of type(int64).max
.
Init
event
Topic | Events | |
Status | Fixed | |
Quality Impact | Medium |
The values for epochSeconds
and uncollectedEpoch
may be beneficial for off-chain consumers. Consider adding these to the Init
event.
Topic | Documentation | |
Status | Fixed | |
Quality Impact | Low |
The following comment is inaccurate:
/// @dev Second param required, but currently unused.
Consider updating the comment accordingly.
Topic | User Experience | |
Status | Fixed | |
Quality Impact | Low |
Calling fulfil()
is called after the pending subscription’s end
timestamp causes the transaction to revert. No harm is done in this scenario except perhaps to user/developer experience.
Consider adding a validation check against the underflow to provide a useful error message. Note that this is likely uncommon case.
require()
Topic | Redundant Code | |
Status | Fixed | |
Gas Savings | Low |
msg.sender
can never be zero in unsubscribe()
. Consider removing this check.
Topic | Redundant Code | |
Status | Fixed | |
Gas Savings | Low |
In fulfil()
, minAmount
is assigned to the same value as amountUsed
. Consider consolidating them into one variable.
collect()
Topic | Storage I/O | |
Status | Fixed | |
Gas Savings | Medium |
Repeated reads and writes to the same storage is discounted, but still expensive. Consider assigning uncollectedEpoch
to a new local variable for use within the collect()
loop, and then writing its value back to uncollectedEpoch
after the loop completes.
This reduces n
storage writes to 1 storage write, saving gas.
unsubscribe()
Topic | Redundant Code | |
Status | Fixed | |
Gas Savings | Low |
In unsubscribe()
:
sub.end > _now
)if
/ else if
can be simplified to an if
/ else
setEpochs(sub.start, sub.end, -int128(sub.rate));
can be moved out and above the if
/ else
statements, as it should occur in both cases.Topic | Input Validation | |
Impact | Informational ✳ |
As described in TMAAR, the responsibility of setting the correct subscription rate is solely on the user. When working with this smart contract:
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 The Graph 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.