Skip to content
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.

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

Fix Staking/Unstaking Bugs & Security Improvements #347

Merged
merged 9 commits into from
Sep 21, 2024

Conversation

Jonny-Ringo
Copy link
Contributor

.prepend finalize handler and add currentHeight = nil finalization rule
The handler was not seeing block heights at the bottom of the stack and could not execute.

Set UnstakeDelay = 670 as a Global

Removing UnstakeDelay as an option on messages removes the risk of it being manually overriden(or set to 1).

Without this, anyone can continuously stake, vote, unstake, and then send to another address and repeat the process. this leaves voting as insecure and is a loophole in the security model.

`.prepend` finalize handler and add `currentHeight = nil` finalization rule
Set UnstakingDelay = 670 as a Global

Removing this as an option on messages removes the risk of it being manually overriden(or set to 1). 

Without this, anyone can continuously stake, vote, unstake, and then send to another address and repeat the process. this leaves voting as insecure and is a loophole in the security model.

This change along with the previous update to .prepend the finalization Handler are included.
if unstakeInfo.release_at == nil or currentHeight >= unstakeInfo.release_at then
local height = tonumber(msg['Block-Height'])
assert(Balances[msg.From] and bint(Balances[msg.From]) >= quantity, "Insufficient balance to stake")
Balances[msg.From] = utils.subtract(Balances[msg.From], msg.Tags.Quantity)
Stakers[msg.From] = Stakers[msg.From] or { amount = "0" }
Stakers[msg.From].amount = utils.add(Stakers[msg.From].amount, msg.Tags.Quantity)
Stakers[msg.From].unstake_at = height + delay
Stakers[msg.From].unstake_at = height + UnstakeDelay
print("Successfully Staked " .. msg.Tags.Quantity)
msg.reply({ Data = "Successfully Staked " .. msg.Tags.Quantity})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we need to check if .reply is supported here

@twilson63
Copy link
Collaborator

twilson63 commented Sep 20, 2024

Are we sure we want to force all users of this staking blueprint to use the staking delay setting of 670? It seems it is very explicit this was implemented allowing a staker to choose their staking delay, I understand your concern about being zero, but we should check with others

@Jonny-Ringo
Copy link
Contributor Author

Are we sure we want to force all users of this staking blueprint to use the staking delay setting of 670? It seems it is very explicit this was implemented allowing a staker to choose their staking delay, I understand your concern about being zero, but we should check with others

The main reason is to prevent someone from continuously staking/voting/unstaking and repeating the process. Without hard-coding some kind of safety on the staking lua this can be tampered with. Ideally it should be UnstakeDelay = VoteLength but that would only work for voting and not other staking usecases so I left it as a 24hr default. Maybe I can notate how to change this since it is really easy to update? Making it a Global also makes it easy to update with DAO votes.

@twilson63 twilson63 merged commit af92119 into permaweb:main Sep 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants