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

Staking code, deployment scripts, tests, and thread model #251

Merged
merged 9 commits into from
Oct 29, 2024

Conversation

drinkcoffee
Copy link
Contributor

@drinkcoffee drinkcoffee commented Oct 18, 2024

Adds a staking contract that has the following functionality:

  • stake some native IMX
  • unstake some or all of the native IMX previously staked
  • distribute some native IMX to one or more of the stakers

This is a work in progress PR.
Done:

  • Design documentation / usage guide in README.md
  • Contract
  • Test plan
  • Test code
  • Deployment script
  • Threat model

Copy link

openzeppelin-code bot commented Oct 18, 2024

[WIP] Staking code, deployment scripts, tests, and thread model

Generated at commit: 8319a53ab923890614d2bfd853a6ee2b41c99694

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
3
0
0
11
27
41
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@drinkcoffee drinkcoffee changed the title [WIP] Staking code, deployment scripts, tests, and thread model Staking code, deployment scripts, tests, and thread model Oct 24, 2024
@drinkcoffee drinkcoffee marked this pull request as ready for review October 24, 2024 01:57
}

/// @notice The amount of value owned by each staker
// solhint-disable-next-line private-vars-leading-underscore

Choose a reason for hiding this comment

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

Why disable this one? If we never adhere this rule, should we turn it off globally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turning this off globally is probably what we should do. However, let's not do it in this PR.
Using _ for variables that are supposed to be private is so that it is obvious if someone mistakenly makes a variable public.
For me, it is a stylistic thing which I don't like. When I see _var I automatically assume it is a function parameter. Most Solidity written pre-circa 2020 used that style. I assume many other people who have used Solidity for a long time also have that reaction.

@@ -1,6 +1,7 @@
@openzeppelin/contracts/=lib/openzeppelin-contracts-4.9.3/contracts/
openzeppelin-contracts-5.0.2/=lib/openzeppelin-contracts-5.0.2/contracts/
openzeppelin-contracts-upgradeable-4.9.3/=lib/openzeppelin-contracts-upgradeable-4.9.3/contracts/
openzeppelin-contracts-4.9.3/=lib/openzeppelin-contracts-4.9.3/contracts/

Choose a reason for hiding this comment

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

Is it intentional to need 4.9.3 when we already reference 5.0.2? If yes, i'm curious as to why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is @openzeppelin/contracts is referenced in Open Zeppelin upgradeable contracts. At some point, we need to upgrade all contracts to the new version of Open Zeppelin, and at that time change @OpenZeppelin to point to 5.0.2 (or the latest that will work on our chain at that time).

/// @notice Amount of stake.
uint256 stake;
/// @notice True if this account has ever staked.
bool hasStaked;

Choose a reason for hiding this comment

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

just reading the variable name hasStaked seemed to me like 'if the user has currently staked or not' only after reading the comment it made it clear for me. What is the purpose of knowing hasStaked? Is it more like history? When the user unstakes completely does this struct become {stake: 0, hasStaked: true}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the user unstakes completely does this struct become {stake: 0, hasStaked: true}?
Correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of hasStaked is to ensure the stakers array has a list of unique addresses: that is, there are no duplicate addresses. If a staker completely unstakes (stake == 0) and then restakes (stake != 0), having hasStaked allows the _addStake function on line 230 to know that the staker's address is already in the stakers array.

function _addStake(address _account, uint256 _amount, bool _existingAccountsOnly) private {
StakeInfo storage stakeInfo = balances[_account];
uint256 currentStake = stakeInfo.stake;
if (!stakeInfo.hasStaked) {

Choose a reason for hiding this comment

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

in javascript/typescript world, if line 231 returned nothing (for a new account), like 233 would error. Interesting, it works fine in Solidity :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Solidity is different. Everything has a default, uninitialised value.

uint256 newBalance = currentStake - _amountToUnstake;
stakeInfo.stake = newBalance;

emit StakeRemoved(msg.sender, _amountToUnstake, newBalance);

Choose a reason for hiding this comment

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

out of curiosity, why emit first and then transfer? or it doesn't matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this emit prior to the transfer as a result of a warning from slither. My guess at why this is deemed better is that there would be two StakeRemoved events emitted if the transfer caused the code to re-enter the function, which would make it easier to detect.
However, there is a test for re-entrancy, and given transfer only supplies 3000 gas, even a minimal re-entrancy attack contract is unable to successfully execute an attack.

@drinkcoffee drinkcoffee merged commit f4eb3e3 into main Oct 29, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants