Staking vulnerability: withdraw delay issue

[Zena-park]

Describe the bug

• Community member, Black Cow, identified overflow attack for withdraw delay that allows the operator to basically reduce the withdraw delay time to 0, effectively removing the withdraw delay time

Configuration

• Severity: High

Impact

1. The withdrawalDelay of the layer2 of the Operator can be set without limitation, and by exploiting this part, an overflow attack is possible, so the unstake+withdraw period can be changed to 0 or a value close to it.
2. Since the Operator can change the withdrawalDelay to any value at any time, an attack that can tie up the assets staked in the layer2 almost indefinitely (for 10^32 years) is also possible.

Recommendation

• Modified the setwithdrawalDelay function to limit it to one month (available only to operators)

Exploit Scenario

Demo

[Zena-park]

deployment on sepolia

npx hardhat run scripts/33-issue-setwithdrawalDelay/deploy/0.deploy.js --network sepolia DepositManager_setWithdrawalDelay 0x7E459BC07736f735E3E758fbB740c385FB4d466B

Apply to depositManager on sepolia

• DepositManagerProxy :
  • function (setWithdrawalDelay(address,uint256)) selector : 0xdc5a709f
  • implementation (index = 2 ) : address(0) → Use the logic of index 2.
  • transaction : setImplementation2 ( 0x7E459BC07736f735E3E758fbB740c385FB4d466B, 2, true)
  • transaction: setSelectorImplementations2([0xdc5a709f],0x7E459BC07736f735E3E758fbB740c385FB4d466B)
  • globalWithdrawalDelay : 93046
  • MAX_DELAY_BLOCKS : 216000; // 60*60*24*30/12 = 216000 (1 block = 12 sec)

Test on sepolia

• setWithdrawalDelay(address l2chain,uint256 withdrawalDelay_) : set as 216000 blocks
  • transaction

[suahnkim]

setWithdrawalDelayByOwner function, which lets owner to set the withdrawal delay, will be activated only if we find that an operator has abused this feature prior to the agenda is passed.

Simple staking is already patched so that the "Unstake" button is deactivated if it detects that the vulnerability is abused.

[suahnkim]

DAO agenda to resolve this issue:

Limit withdrawal delay period by operator

Simple Summary Fix overflow vulnerability and limit Operator's ability to set custom withdrawal delay period to at most 30 days.

Motivation Community member, Black Cow, identified overflow attack for withdraw delay that allows the operator to basically reduce the withdraw delay time to 0, effectively removing the withdraw delay time. Tokamak Network contract team verified the result, and also identified that the function can be abused to lock staked TON for very long time (~10^32 years), permanently locking staked TON.

Specification This proposal will limit operator account's ability of setting withdraw delay for their layer2 to maximum of 216000 blocks (~ 30 days).

References

• Implementation: setWithdrawalDelay
• Tests: Sepolia
• Discussion: Github issue
• Bounty: will be updated later

Copyright Copyright and related rights waived via CC0.

[zzooppii]

develop test and script for CreateAgenda on Sepolia and Mainnet (issue)

createAgenda Tx : https://etherscan.io/tx/0x31d1370bad392ad46f79452be3d40126fd8f00a6f65175faf94ffd0073097cf8

AgendaID : 11 getAgendaNoticeEndTimeSeconds : 1723796759 (24년 8월 16일 오후 5시 26분) Agenda Page : https://dao.tokamak.network/#/agenda/11

[blackcow1987]

I think it would be good if the withdrawal logic also had some delay like the adjustCommissionDelay. https://github.com/tokamak-network/ton-staking-v2/blob/main/contracts/stake/managers/SeigManager.sol#L615

[Zena-park]

I think it would be good if the withdrawal logic also had some delay like the adjustCommissionDelay. https://github.com/tokamak-network/ton-staking-v2/blob/main/contracts/stake/managers/SeigManager.sol#L615

In terms of service, it seems to be a good function. I will share it with internal developers to hear their opinions and let you know.

[suahnkim]

this my personal opinion: I think setWithdrawDelay and adjustCommissionDelay have two different goals:

• adjustComissionDelay -> this is to make the Operator rich, it benefits the Operator, thus adding a delay effect gives users to withdraw from the layer before the effect takes place.
• setWithdrawDelay -> this is to ensure some security within their layer (although it is not implemented at the moment), thus adding a delay effect may interfere with its security mechanics, and probably should not give users time to withdraw from the layer (especially if a malicious actor that is trying to bypass some security)

-> in conclusion, I don't think we should add delay effect to setWithdrawDelay

[blackcow1987]

@suahnkim I think the problem can be solved by keeping "setWithdrawalDelayByOwner" method to change the withdrawal delay without delay. If there is a problem in Layer2, it seems more appropriate to call 'setWithdrawalDelayByOwner' method through DAO proposal rather than operator changing delay arbitrarily without the consent of the users.

[suahnkim]

@blackcow1987

1. setWithdrawDelayByOwner will be only available if we detect the abuse before this agenda gets passed, otherwise it will not be available for execution via DAO (although we can easily add the functionality back)
2. If setWithdrawDelayByOwner is available for DAO via agenda execution, it will take a while for setWithdrawDelayByOwner will take effect due to DAO agenda execution time -> even without adding a delay, it will have a delay effect

I am not convinced that delay effect is necessary if setWithdrawDelay is used correctly by Operator, and if they use it incorrectly, it will simply add 16 more days to existing withdraw delay time.

If delay effect is added to setWithdrawDelay(), what is your recommended delay effect time?

[blackcow1987]

@suahnkim

1. I can't assume that operators will always use the setWithdrawDelay method correctly.
2. I agree that setWithdrawDelayByOwner should not be executed as a DAO agenda, because someone could abuse it.
3. However, I still think that when there is a problem in Layer 2, it would be more appropriate to go through a process such as stopping withdrawals through DAO agenda rather than a withdrawal delay. Since the minimum withdrawal delay is about 2 weeks, I think there is enough room to process the DAO agenda.
4. I haven't thought about how long the delay should be, but I think it would be good to set it to a configurable value like the commission rate and set the initial value of the delay to the same as the commission rate delay.

The goal of this opinion is that when there is an event such as a change in withdrawal period, users need time to react to the change.

[suahnkim]

@blackcow1987 I understand your point of view, but I disagree.

If there is a security assumption where withdrawDelay has to be set right away, it may be dangerous to have a delay effect. The negative of allowing setting withdrawDelay without a delay effect is far less than the increase of 16 days to withdraw for users, from my point of view.

Please remember that commission is beneficial for Operators, whereas withdrawDelay can affect the whole ecosystem (although this may depend on how our staked TON is used in the future, ex: bonding using staked TON for state root verification or challenge mechanics)

-> this is just my personal opinion.

[blackcow1987]

@suahnkim Thank you for sharing your opinion :) I agree that the change to withdrawal delay has very limited impact for users. I'll leave the decisions up to the team !

[suahnkim]

@blackcow1987 since we do not know the exact terms of the security assumptions right now,

one compromise we can have is

1. include delay effect to setWithdrawDelay with adjustWithdrawDelay (onlyOwner function) as you have proposed
2. Once we have staked TON utility, such as bonding using staked TON for verification + challenge, slashing mechanism implemented, we can change adjustWithdrawDelay to 0 using DAO agenda -> effectively removing the delay effect (or just leave it as is if there are no issues)

This way, I think your concerns are met and we can prevent any security issues in the future if we need to.

[suahnkim]

Anyways, we are always thankful, and appreciate your comments and concerns Thanks @blackcow1987

[zzooppii]

@suahnkim @blackcow1987 First of all, I agree with Suah's opinion.

The reason I agree with Suah's opinion is that using setWithdrawDelay is the operator's right.

However, even if you have the right, you can't use it as you please. This is because if there is a lot of staking in the operator's layer, the operator will benefit. If the operator uses setWithdrawDelay at will, all the users there will leave and move to other layers, which will ultimately result in the operator suffering a loss.

Therefore, using setWithdrawDelay is the operator's right, and it is also the operator's responsibility to suffer losses by using it without reason.

Thank you for always giving BlockCow good opinions.

[suahnkim]

@blackcow1987 Our internal team had a discussion about your request.

• We want to look into adding the delay effect to setWithdrawDelay() as you have requested when we build on top of our staking service (the development may not happen right away, as we are still in middle of discussing some ideas, but we will update you on the progress in this issue).
• We will also patch the frontend to account for custom withdrawdelay[layer2] and show it.

We are thankful for your opinion and support and look forward to your vote on this agenda.

If you have any questions or other concerns, feel free to share them.

Sincerely, Tokamak Network contract team

[blackcow1987]

@suahnkim Thank you to the Tokamak Network contract team for considering my opinion :)

[suahnkim]

@blackcow1987 3,300 TON has been paid as bounty

This will be recorded in the description under DAO agenda as well.

[suahnkim]

Please note that this issue will be closed when DAO agenda is successfully passed.