Skip to content

[audit-12] Fix Recommendations. #1209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: ma/indexing-payments-audit-fixes-11-L-9
Choose a base branch
from
Open

[audit-12] Fix Recommendations. #1209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6d9a18c
fix: [TRST-R-1] minor fixes
matiasedgeandnode Jul 24, 2025
58b7a28
fix: [TRST-R-4] CEI violation
matiasedgeandnode Jul 24, 2025
5f732ac
fix: [TRST-R-5] Terms validation
matiasedgeandnode Jul 24, 2025
b492251
fix: [TRST-R-6] Configurable indexing fees cut
matiasedgeandnode Jul 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/horizon/contracts/payments/collectors/RecurringCollector.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC

/**
* @inheritdoc IRecurringCollector
* @notice Accept an indexing agreement.
* @notice Accept a Recurring Collection Agreement.
* See {IRecurringCollector.accept}.
* @dev Caller must be the data service the RCA was issued to.
*/
Expand Down Expand Up @@ -148,7 +148,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC

/**
* @inheritdoc IRecurringCollector
* @notice Cancel an indexing agreement.
* @notice Cancel a Recurring Collection Agreement.
* See {IRecurringCollector.cancel}.
* @dev Caller must be the data service for the agreement.
*/
Expand Down Expand Up @@ -181,7 +181,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC

/**
* @inheritdoc IRecurringCollector
* @notice Update an indexing agreement.
* @notice Update a Recurring Collection Agreement.
* See {IRecurringCollector.update}.
* @dev Caller must be the data service for the agreement.
* @dev Note: Updated pricing terms apply immediately and will affect the next collection
Expand Down Expand Up @@ -343,7 +343,10 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
slippage <= _params.maxSlippage,
RecurringCollectorExcessiveSlippage(_params.tokens, tokensToCollect, _params.maxSlippage)
);
}
agreement.lastCollectionAt = uint64(block.timestamp);

if (tokensToCollect > 0) {
_graphPaymentsEscrow().collect(
_paymentType,
agreement.payer,
Expand All @@ -354,7 +357,6 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
_params.receiverDestination
);
}
agreement.lastCollectionAt = uint64(block.timestamp);

emit PaymentCollected(
_paymentType,
Expand Down
10 changes: 9 additions & 1 deletion packages/subgraph-service/contracts/SubgraphService.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,13 @@ contract SubgraphService is
emit CurationCutSet(curationCut);
}

/// @inheritdoc ISubgraphService
function setIndexingFeesCut(uint256 indexingFeesCut_) external override onlyOwner {
require(PPMMath.isValidPPM(indexingFeesCut_), SubgraphServiceInvalidIndexingFeesCut(indexingFeesCut_));
indexingFeesCut = indexingFeesCut_;
emit IndexingFeesCutSet(indexingFeesCut_);
}

/**
* @inheritdoc ISubgraphService
* @notice Accept an indexing agreement.
Expand Down Expand Up @@ -793,7 +800,8 @@ contract SubgraphService is
agreementId: _agreementId,
currentEpoch: _graphEpochManager().currentEpoch(),
receiverDestination: _paymentsDestination,
data: _data
data: _data,
indexingFeesCut: indexingFeesCut
})
);

Expand Down
3 changes: 3 additions & 0 deletions packages/subgraph-service/contracts/SubgraphServiceStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,7 @@ abstract contract SubgraphServiceV1Storage {

/// @notice Destination of indexer payments
mapping(address indexer => address destination) public paymentsDestination;

/// @notice The cut data service takes from indexing fee payments. In PPM.
uint256 public indexingFeesCut;
}
19 changes: 19 additions & 0 deletions packages/subgraph-service/contracts/interfaces/ISubgraphService.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,24 @@ interface ISubgraphService is IDataServiceFees {
*/
event CurationCutSet(uint256 curationCut);

/**
* @notice Emitted when indexing fees cut is set
* @param indexingFeesCut The indexing fees cut
*/
event IndexingFeesCutSet(uint256 indexingFeesCut);

/**
* @notice Thrown when trying to set a curation cut that is not a valid PPM value
* @param curationCut The curation cut value
*/
error SubgraphServiceInvalidCurationCut(uint256 curationCut);

/**
* @notice Thrown when trying to set an indexing fees cut that is not a valid PPM value
* @param indexingFeesCut The indexing fees cut value
*/
error SubgraphServiceInvalidIndexingFeesCut(uint256 indexingFeesCut);

/**
* @notice Thrown when an indexer tries to register with an empty URL
*/
Expand Down Expand Up @@ -252,6 +264,13 @@ interface ISubgraphService is IDataServiceFees {
*/
function setCurationCut(uint256 curationCut) external;

/**
* @notice Sets the data service payment cut for indexing fees
* @dev Emits a {IndexingFeesCutSet} event
* @param indexingFeesCut The indexing fees cut for the payment type
*/
function setIndexingFeesCut(uint256 indexingFeesCut) external;

/**
* @notice Sets the payments destination for an indexer to receive payments
* @dev Emits a {PaymentsDestinationSet} event
Expand Down
44 changes: 40 additions & 4 deletions packages/subgraph-service/contracts/libraries/IndexingAgreement.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,15 @@ library IndexingAgreement {
* @param currentEpoch The current epoch
* @param receiverDestination The address where the collected fees should be sent
* @param data The encoded data containing the number of entities indexed, proof of indexing, and epoch
* @param indexingFeesCut The indexing fees cut in PPM
*/
struct CollectParams {
address indexer;
bytes16 agreementId;
uint256 currentEpoch;
address receiverDestination;
bytes data;
uint256 indexingFeesCut;
}

/**
Expand Down Expand Up @@ -271,6 +273,13 @@ library IndexingAgreement {
*/
error IndexingAgreementNotAuthorized(bytes16 agreementId, address unauthorizedIndexer);

/**
* @notice Thrown when indexing agreement terms are invalid
* @param tokensPerSecond The indexing agreement tokens per second
* @param maxOngoingTokensPerSecond The RCA maximum tokens per second
*/
error IndexingAgreementInvalidTerms(uint256 tokensPerSecond, uint256 maxOngoingTokensPerSecond);

/**
* @notice Accept an indexing agreement.
*
Expand Down Expand Up @@ -343,7 +352,7 @@ library IndexingAgreement {
agreement.allocationId = allocationId;

require(metadata.version == IndexingAgreementVersion.V1, IndexingAgreementInvalidVersion(metadata.version));
_setTermsV1(self, agreementId, metadata.terms);
_setTermsV1(self, agreementId, metadata.terms, signedRCA.rca.maxOngoingTokensPerSecond);

emit IndexingAgreementAccepted(
signedRCA.rca.serviceProvider,
Expand Down Expand Up @@ -392,7 +401,12 @@ library IndexingAgreement {

require(wrapper.agreement.version == IndexingAgreementVersion.V1, "internal: invalid version");
require(metadata.version == IndexingAgreementVersion.V1, IndexingAgreementInvalidVersion(metadata.version));
_setTermsV1(self, signedRCAU.rcau.agreementId, metadata.terms);
_setTermsV1(
self,
signedRCAU.rcau.agreementId,
metadata.terms,
wrapper.collectorAgreement.maxOngoingTokensPerSecond
);

emit IndexingAgreementUpdated({
indexer: wrapper.collectorAgreement.serviceProvider,
Expand Down Expand Up @@ -565,7 +579,7 @@ library IndexingAgreement {
agreementId: params.agreementId,
collectionId: bytes32(uint256(uint160(wrapper.agreement.allocationId))),
tokens: expectedTokens,
dataServiceCut: 0,
dataServiceCut: params.indexingFeesCut,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will send the cut to the subgraph service contract, which would require a contract update for those funds to be used/transferred, right? (Not a problem per se, but maybe worth documenting)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... I believe this mimics what's done for query fees. Is that the case for query fees as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With query fees all of the data service fees are redistributed (as curation fees)

receiverDestination: params.receiverDestination,
maxSlippage: data.maxSlippage
})
Expand Down Expand Up @@ -621,9 +635,16 @@ library IndexingAgreement {
* @param _manager The indexing agreement storage manager
* @param _agreementId The id of the agreement to update
* @param _data The encoded terms data
* @param maxOngoingTokensPerSecond The RCA maximum tokens per second limit for validation
*/
function _setTermsV1(StorageManager storage _manager, bytes16 _agreementId, bytes memory _data) private {
function _setTermsV1(
StorageManager storage _manager,
bytes16 _agreementId,
bytes memory _data,
uint256 maxOngoingTokensPerSecond
) private {
IndexingAgreementTermsV1 memory newTerms = IndexingAgreementDecoder.decodeIndexingAgreementTermsV1(_data);
_validateTermsAgainstRCA(newTerms, maxOngoingTokensPerSecond);
_manager.termsV1[_agreementId].tokensPerSecond = newTerms.tokensPerSecond;
_manager.termsV1[_agreementId].tokensPerEntityPerSecond = newTerms.tokensPerEntityPerSecond;
}
Expand Down Expand Up @@ -764,4 +785,19 @@ library IndexingAgreement {
collectorAgreement: _directory().recurringCollector().getAgreement(agreementId)
});
}

/**
* @notice Validates indexing agreement terms against RCA limits
* @param terms The indexing agreement terms to validate
* @param maxOngoingTokensPerSecond The RCA maximum tokens per second limit
*/
function _validateTermsAgainstRCA(
IndexingAgreementTermsV1 memory terms,
uint256 maxOngoingTokensPerSecond
) private pure {
require(
terms.tokensPerSecond <= maxOngoingTokensPerSecond,
IndexingAgreementInvalidTerms(terms.tokensPerSecond, maxOngoingTokensPerSecond)
);
}
}
2 changes: 1 addition & 1 deletion packages/subgraph-service/contracts/libraries/IndexingAgreementDecoder.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ library IndexingAgreementDecoder {
) {
return decoded;
} catch {
revert IndexingAgreementDecoderInvalidData("decodeCollectIndexingFeeData", data);
revert IndexingAgreementDecoderInvalidData("decodeIndexingAgreementTermsV1", data);
}
}
}
2 changes: 1 addition & 1 deletion packages/subgraph-service/test/unit/subgraphService/indexing-agreement/accept.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ contract SubgraphServiceIndexingAgreementAcceptTest is SubgraphServiceIndexingAg

bytes memory expectedErr = abi.encodeWithSelector(
IndexingAgreementDecoder.IndexingAgreementDecoderInvalidData.selector,
"decodeCollectIndexingFeeData",
"decodeIndexingAgreementTermsV1",
invalidTermsData
);
vm.expectRevert(expectedErr);
Expand Down
2 changes: 1 addition & 1 deletion packages/subgraph-service/test/unit/subgraphService/indexing-agreement/shared.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ contract SubgraphServiceIndexingAgreementSharedTest is SubgraphServiceTest, Boun
);
rcau.metadata = _encodeUpdateIndexingAgreementMetadataV1(
_newUpdateIndexingAgreementMetadataV1(
_ctx.ctxInternal.seed.termsV1.tokensPerSecond,
bound(_ctx.ctxInternal.seed.termsV1.tokensPerSecond, 0, _rca.maxOngoingTokensPerSecond),
_ctx.ctxInternal.seed.termsV1.tokensPerEntityPerSecond
)
);
Expand Down