Skip to content

TRST audit fixes for HorizonStaking contract #1073

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

Merged
merged 23 commits into from
Dec 17, 2024
Merged

TRST audit fixes for HorizonStaking contract #1073

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
ce749be
fix: separate delegation and provision thaw request lists (TRST-H02)
Maikol Nov 28, 2024
d02f410
fix: separate delegation and delegationWithBeneficiary thaw requests …
Maikol Nov 29, 2024
167055a
fix: round down tokens thawing when slashing (TRST-H04)
Maikol Dec 2, 2024
abe3321
fix: add legacy slasher for transition period (TRST-H06)
Maikol Dec 3, 2024
f254897
fix: add missing legacy withdraw delegated (TRST-H07)
Maikol Dec 4, 2024
91cda56
fix: added minimum delegation for provisions (TRST-M03)
Maikol Dec 4, 2024
07ef418
fix: operator check in closeAllocation (TRST-M12)
Maikol Dec 4, 2024
26e4dc7
fix: getThawedTokens calculation (TRST-L03)
Maikol Dec 4, 2024
c9f8a2f
fix: documentation on unstake (TRST-M11)
Maikol Dec 4, 2024
9271b99
fix: remove unused value from event (TRST-R01)
Maikol Dec 4, 2024
151e63a
fix: round thawing shares up (TRST-R07)
Maikol Dec 5, 2024
6e5a295
fix: check shares are not zero when creating a thaw request (TRST-R14)
Maikol Dec 5, 2024
d9c6190
fix: move legacyWithdrawDelegated to withdrawDelegated (TRST-H07)
Maikol Dec 6, 2024
161f8a2
fix: added comment for stack too deep solution
Maikol Dec 6, 2024
43bc72b
fix: added comment to explain minimum delegation (TRST-M03)
Maikol Dec 6, 2024
c59c186
fix: new event for minimum delegation not met (TRST-M03)
Maikol Dec 6, 2024
d1c5cc7
fix: add a new mapping instead of splitting thaw requests (TRST-H02)
Maikol Dec 6, 2024
6e00d17
docs: fix documentation errors (TRST-R09)
tmigone Dec 9, 2024
d6d376c
fix: legacy slashing underflow (TRST-H08)
Maikol Dec 12, 2024
7d90ad2
fix: underflow in getIdleStake (TRST-L14)
Maikol Dec 12, 2024
57aea44
fix: added thaw request type to thaw request fulfilled event (TRST-R15)
Maikol Dec 12, 2024
bbd23f5
fix: add minimum tokens amount for undelegate with beneficiary (TRST-…
Maikol Dec 13, 2024
399b7a9
fix: natspec for new undelegate error
Maikol Dec 13, 2024
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
11 changes: 11 additions & 0 deletions packages/horizon/contracts/staking/HorizonStaking.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
/// @dev Address of the staking extension contract
address private immutable STAKING_EXTENSION_ADDRESS;

/// @dev Minimum amount of delegation.
uint256 private constant MIN_DELEGATION = 1e18;

/**
* @notice Checks that the caller is authorized to operate over a provision.
* @param serviceProvider The address of the service provider.
Expand Down Expand Up @@ -842,6 +845,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
* have been done before calling this function.
*/
function _delegate(address _serviceProvider, address _verifier, uint256 _tokens, uint256 _minSharesOut) private {
require(_tokens >= MIN_DELEGATION, HorizonStakingInsufficientTokens(_tokens, MIN_DELEGATION));
require(
_provisions[_serviceProvider][_verifier].createdAt != 0,
HorizonStakingInvalidProvision(_serviceProvider, _verifier)
Expand Down Expand Up @@ -911,6 +915,13 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {

pool.shares = pool.shares - _shares;
delegation.shares = delegation.shares - _shares;
if (delegation.shares != 0) {
uint256 remainingTokens = (delegation.shares * (pool.tokens - pool.tokensThawing)) / pool.shares;
require(
remainingTokens >= MIN_DELEGATION,
HorizonStakingInsufficientTokens(remainingTokens, MIN_DELEGATION)
);
}

bytes32 thawRequestId = _createThawRequest(
_requestType,
Expand Down
2 changes: 1 addition & 1 deletion packages/horizon/test/escrow/collect.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ contract GraphEscrowCollectTest is GraphEscrowTest {
uint256 tokensDelegatoion = tokens * delegationFeeCut / MAX_PPM;
vm.assume(tokensDataService < tokens - tokensProtocol - tokensDelegatoion);

delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);
resetPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);

Expand Down
2 changes: 1 addition & 1 deletion packages/horizon/test/payments/GraphPayments.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ contract GraphPaymentsTest is HorizonStakingSharedTest {
address escrowAddress = address(escrow);

// Delegate tokens
tokensDelegate = bound(tokensDelegate, 1, MAX_STAKING_TOKENS);
tokensDelegate = bound(tokensDelegate, MIN_DELEGATION, MAX_STAKING_TOKENS);
vm.startPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, tokensDelegate, 0);

Expand Down
28 changes: 27 additions & 1 deletion packages/horizon/test/staking/HorizonStaking.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ contract HorizonStakingTest is HorizonStakingSharedTest {
modifier useDelegation(uint256 delegationAmount) {
address msgSender;
(, msgSender, ) = vm.readCallers();
vm.assume(delegationAmount > 1);
vm.assume(delegationAmount >= MIN_DELEGATION);
vm.assume(delegationAmount <= MAX_STAKING_TOKENS);
vm.startPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, delegationAmount, 0);
Expand All @@ -56,4 +56,30 @@ contract HorizonStakingTest is HorizonStakingSharedTest {
resetPrank(msgSender);
_;
}

modifier useUndelegate(uint256 shares) {
resetPrank(users.delegator);

DelegationPoolInternalTest memory pool = _getStorage_DelegationPoolInternal(
users.indexer,
subgraphDataServiceAddress,
false
);
DelegationInternal memory delegation = _getStorage_Delegation(
users.indexer,
subgraphDataServiceAddress,
users.delegator,
false
);

shares = bound(shares, 1, delegation.shares);
uint256 tokens = (shares * (pool.tokens - pool.tokensThawing)) / pool.shares;
if (shares < delegation.shares) {
uint256 remainingTokens = (shares * (pool.tokens - pool.tokensThawing - tokens)) / pool.shares;
vm.assume(remainingTokens >= MIN_DELEGATION);
}

_undelegate(users.indexer, subgraphDataServiceAddress, shares);
_;
}
}
5 changes: 3 additions & 2 deletions packages/horizon/test/staking/delegation/addToPool.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ contract HorizonStakingDelegationAddToPoolTest is HorizonStakingTest {

modifier useValidDelegationAmount(uint256 tokens) {
vm.assume(tokens <= MAX_STAKING_TOKENS);
vm.assume(tokens >= MIN_DELEGATION);
_;
}

Expand Down Expand Up @@ -93,7 +94,7 @@ contract HorizonStakingDelegationAddToPoolTest is HorizonStakingTest {
uint256 recoverAmount
) public useIndexer useProvision(tokens, 0, 0) useDelegationSlashing() {
recoverAmount = bound(recoverAmount, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);

// create delegation pool
resetPrank(users.delegator);
Expand All @@ -116,7 +117,7 @@ contract HorizonStakingDelegationAddToPoolTest is HorizonStakingTest {
uint256 recoverAmount
) public useIndexer useProvision(tokens, 0, 0) useDelegationSlashing() {
recoverAmount = bound(recoverAmount, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);

// create delegation pool
resetPrank(users.delegator);
Expand Down
24 changes: 20 additions & 4 deletions packages/horizon/test/staking/delegation/delegate.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,25 @@ contract HorizonStakingDelegateTest is HorizonStakingTest {
staking.delegate(users.indexer, subgraphDataServiceAddress, 0, 0);
}

function testDelegate_RevertWhen_UnderMinDelegation(
uint256 amount,
uint256 delegationAmount
) public useIndexer useProvision(amount, 0, 0) {
delegationAmount = bound(delegationAmount, 1, MIN_DELEGATION - 1);
vm.startPrank(users.delegator);
token.approve(address(staking), delegationAmount);
bytes memory expectedError = abi.encodeWithSelector(
IHorizonStakingMain.HorizonStakingInsufficientTokens.selector,
delegationAmount,
MIN_DELEGATION
);
vm.expectRevert(expectedError);
staking.delegate(users.indexer, subgraphDataServiceAddress, delegationAmount, 0);
}

function testDelegate_LegacySubgraphService(uint256 amount, uint256 delegationAmount) public useIndexer {
amount = bound(amount, 1 ether, MAX_STAKING_TOKENS);
delegationAmount = bound(delegationAmount, 1, MAX_STAKING_TOKENS);
delegationAmount = bound(delegationAmount, MIN_DELEGATION, MAX_STAKING_TOKENS);
_createProvision(users.indexer, subgraphDataServiceLegacyAddress, amount, 0, 0);

resetPrank(users.delegator);
Expand All @@ -72,7 +88,7 @@ contract HorizonStakingDelegateTest is HorizonStakingTest {
uint256 tokens,
uint256 delegationTokens
) public useIndexer useProvision(tokens, 0, 0) useDelegationSlashing() {
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);

resetPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
Expand All @@ -96,7 +112,7 @@ contract HorizonStakingDelegateTest is HorizonStakingTest {
uint256 tokens,
uint256 delegationTokens
) public useIndexer useProvision(tokens, 0, 0) useDelegationSlashing() {
delegationTokens = bound(delegationTokens, 2, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION * 2, MAX_STAKING_TOKENS);

resetPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
Expand Down Expand Up @@ -126,7 +142,7 @@ contract HorizonStakingDelegateTest is HorizonStakingTest {
uint256 recoverAmount
) public useIndexer useProvision(tokens, 0, 0) useDelegationSlashing() {
recoverAmount = bound(recoverAmount, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);

// create delegation pool
resetPrank(users.delegator);
Expand Down
13 changes: 0 additions & 13 deletions packages/horizon/test/staking/delegation/redelegate.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,6 @@ import { HorizonStakingTest } from "../HorizonStaking.t.sol";

contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest {

/*
* MODIFIERS
*/

modifier useUndelegate(uint256 shares) {
resetPrank(users.delegator);
DelegationInternal memory delegation = _getStorage_Delegation(users.indexer, subgraphDataServiceAddress, users.delegator, false);
shares = bound(shares, 1, delegation.shares);

_undelegate(users.indexer, subgraphDataServiceAddress, shares);
_;
}

/*
* HELPERS
*/
Expand Down
41 changes: 36 additions & 5 deletions packages/horizon/test/staking/delegation/undelegate.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ contract HorizonStakingUndelegateTest is HorizonStakingTest {
uint256 undelegateSteps
) public useIndexer useProvision(amount, 0, 0) {
undelegateSteps = bound(undelegateSteps, 1, 10);
delegationAmount = bound(delegationAmount, 10 wei, MAX_STAKING_TOKENS);
delegationAmount = bound(delegationAmount, MIN_DELEGATION * undelegateSteps, MAX_STAKING_TOKENS);

resetPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, delegationAmount, 0);
Expand All @@ -44,9 +44,17 @@ contract HorizonStakingUndelegateTest is HorizonStakingTest {
);

uint256 undelegateAmount = delegation.shares / undelegateSteps;
for (uint i = 0; i < undelegateSteps; i++) {
for (uint i = 0; i < undelegateSteps - 1; i++) {
_undelegate(users.indexer, subgraphDataServiceAddress, undelegateAmount);
}

delegation = _getStorage_Delegation(
users.indexer,
subgraphDataServiceAddress,
users.delegator,
false
);
_undelegate(users.indexer, subgraphDataServiceAddress, delegation.shares);
}

function testUndelegate_WithBeneficiary(
Expand All @@ -60,6 +68,29 @@ contract HorizonStakingUndelegateTest is HorizonStakingTest {
_undelegateWithBeneficiary(users.indexer, subgraphDataServiceAddress, delegation.shares, beneficiary);
}

function testUndelegate_RevertWhen_InsuficientTokens(
uint256 amount,
uint256 delegationAmount,
uint256 undelegateAmount
) public useIndexer useProvision(amount, 0, 0) useDelegation(delegationAmount) {
undelegateAmount = bound(undelegateAmount, 1, delegationAmount);
resetPrank(users.delegator);
DelegationInternal memory delegation = _getStorage_Delegation(
users.indexer,
subgraphDataServiceAddress,
users.delegator,
false
);
undelegateAmount = bound(undelegateAmount, delegation.shares - MIN_DELEGATION + 1, delegation.shares - 1);
bytes memory expectedError = abi.encodeWithSelector(
IHorizonStakingMain.HorizonStakingInsufficientTokens.selector,
delegation.shares - undelegateAmount,
MIN_DELEGATION
);
vm.expectRevert(expectedError);
staking.undelegate(users.indexer, subgraphDataServiceAddress, undelegateAmount);
}

function testUndelegate_RevertWhen_TooManyUndelegations()
public
useIndexer
Expand Down Expand Up @@ -112,7 +143,7 @@ contract HorizonStakingUndelegateTest is HorizonStakingTest {

function testUndelegate_LegacySubgraphService(uint256 amount, uint256 delegationAmount) public useIndexer {
amount = bound(amount, 1, MAX_STAKING_TOKENS);
delegationAmount = bound(delegationAmount, 1, MAX_STAKING_TOKENS);
delegationAmount = bound(delegationAmount, MIN_DELEGATION, MAX_STAKING_TOKENS);
_createProvision(users.indexer, subgraphDataServiceLegacyAddress, amount, 0, 0);

resetPrank(users.delegator);
Expand All @@ -131,7 +162,7 @@ contract HorizonStakingUndelegateTest is HorizonStakingTest {
uint256 tokens,
uint256 delegationTokens
) public useIndexer useProvision(tokens, 0, 0) useDelegationSlashing() {
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);

resetPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
Expand Down Expand Up @@ -162,7 +193,7 @@ contract HorizonStakingUndelegateTest is HorizonStakingTest {
uint256 tokens,
uint256 delegationTokens
) public useIndexer useProvision(tokens, 0, 0) useDelegationSlashing {
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);

// delegate
resetPrank(users.delegator);
Expand Down
21 changes: 2 additions & 19 deletions packages/horizon/test/staking/delegation/withdraw.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,6 @@ import { LinkedList } from "../../../contracts/libraries/LinkedList.sol";
import { HorizonStakingTest } from "../HorizonStaking.t.sol";

contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest {
/*
* MODIFIERS
*/

modifier useUndelegate(uint256 shares) {
resetPrank(users.delegator);
DelegationInternal memory delegation = _getStorage_Delegation(
users.indexer,
subgraphDataServiceAddress,
users.delegator,
false
);
shares = bound(shares, 1, delegation.shares);

_undelegate(users.indexer, subgraphDataServiceAddress, shares);
_;
}

/*
* TESTS
Expand Down Expand Up @@ -81,7 +64,7 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest {
}

function testWithdrawDelegation_LegacySubgraphService(uint256 delegationAmount) public useIndexer {
delegationAmount = bound(delegationAmount, 1, MAX_STAKING_TOKENS);
delegationAmount = bound(delegationAmount, MIN_DELEGATION, MAX_STAKING_TOKENS);
_createProvision(users.indexer, subgraphDataServiceLegacyAddress, 10_000_000 ether, 0, MAX_THAWING_PERIOD);

resetPrank(users.delegator);
Expand Down Expand Up @@ -111,7 +94,7 @@ contract HorizonStakingWithdrawDelegationTest is HorizonStakingTest {
uint256 tokens,
uint256 delegationTokens
) public useIndexer useProvision(tokens, 0, MAX_THAWING_PERIOD) useDelegationSlashing() {
delegationTokens = bound(delegationTokens, 2, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION * 2, MAX_STAKING_TOKENS);

resetPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
Expand Down
6 changes: 3 additions & 3 deletions packages/horizon/test/staking/slash/slash.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
uint256 delegationTokens
) public useIndexer useProvision(tokens, MAX_PPM, 0) {
vm.assume(slashTokens > tokens);
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);
verifierCutAmount = bound(verifierCutAmount, 0, MAX_PPM);
vm.assume(verifierCutAmount <= tokens);

Expand All @@ -71,7 +71,7 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
uint256 verifierCutAmount,
uint256 delegationTokens
) public useIndexer useProvision(tokens, MAX_PPM, 0) useDelegationSlashing() {
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);
slashTokens = bound(slashTokens, tokens + 1, tokens + 1 + delegationTokens);
verifierCutAmount = bound(verifierCutAmount, 0, tokens);

Expand Down Expand Up @@ -110,7 +110,7 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
uint256 tokens,
uint256 delegationTokens
) public useIndexer useProvision(tokens, MAX_PPM, 0) useDelegationSlashing {
delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
delegationTokens = bound(delegationTokens, MIN_DELEGATION, MAX_STAKING_TOKENS);

resetPrank(users.delegator);
_delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
Expand Down
1 change: 1 addition & 0 deletions packages/horizon/test/utils/Constants.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ abstract contract Constants {
uint256 internal constant MAX_THAW_REQUESTS = 100;
uint64 internal constant MAX_THAWING_PERIOD = 28 days;
uint32 internal constant THAWING_PERIOD_IN_BLOCKS = 300;
uint256 internal constant MIN_DELEGATION = 1e18;
// Epoch manager
uint256 internal constant EPOCH_LENGTH = 1;
// Rewards manager
Expand Down
Loading