aurora-is-near/aurora-staking-contracts

Aurora staking contracts

aurora-staking-contracts's Issues

change the `isActive` and `isProposed` variables to enum `StreamState`

Other recommendations:

Make cancelStreamProposal not available for active stream (isActive == true) and make stream.schedule.time[0] verification for cancelStreamProposal strict: stream.schedule.time[0] < block.timestamp.

Add a minimum period of time between current timestamp and the start of schedule

_JetStakingV1: function validateStreamParameters().

Currently, the start of rewards schedule must be greater than current block.timestamp(Validation on Line 1095), thus, it is possible to pass scheduleTimes[0] with low difference with current timestamp, so that the creator of stream will not have enough time to activate the stream.

Recommendation. Add a minimum period of time between scheduleTimes[0] and block.timestamp to make sure the stream creator has enough time to activate the stream.

Create a specific role for the whitelisting and blacklisting of the rewards streams

Creating this role would result in separation of the respective functionality from the other admin functions.

Why in the `initialize` function there's no transfer of AURORA tokens for the default stream?

Why in the initialize function there's no transfer of AURORA tokens for the default stream?

Originally posted by @alexauroradev in #2 (comment)

remove voting tokens from the staking contract

update deployment scripts with the proper roles.

Aurora Auto Compounding. How it works?

I was researching on the auto compounding mechanisms on the Aurora Staking Contract. What I understand is the staking service increases total staked aurora, so the newer participants will have lower shares of the aurora's staked, resulting in something of auto compounding.

My question is this: The Math is quite subtle to understand, so I do not really know if I am correct. I just wanted some points on how exactly Auto Compounding works, plus could you provide me to some codes that does this.

Missing validation

Description

According to the documentation, stake, unstake, unstakeAll, stakeOnBehalfOfOtherUsers, stakeOnBehalfOfAnotherUser functions should claim rewards if the selected user has actual staking.
Mentioned validation should be implemented to prevent human factors. Contract: JetStakingV1

Functions: initialize, updateTreasury, stake, unstake, unstakeAll, stakeOnBehalfOfOtherUsers, stakeOnBehalfOfAnotherUser

Recommendation: implement these checks.

get stream manager address

aurora-staking-contracts/contracts/JetStakingV1.sol

Line 487 in 5b8bc9a

function getStream(uint256 streamId)

Improve events information.

https://github.com/aurora-is-near/aurora-staking-contracts/pull/90/files#r864339130

update Tau value in the deployment and tasks script

• tau for main AURORA stream is one month
• tau for other stream is one day.

Validate `tau` parameter

Description:

JetStakingV1: function _validateStreamParameters().
Parameter “tau” should be validated in order not to be equal to extremely big values, due to which users won’t be able to withdraw their pending rewards.

Recommendation.

• Validate that “tau” is not equal to big value and doesn’t block users from withdrawing their pending rewards.

Solc-0.8.10 is not recommended for deployment

Consider v0.8.7 for deployment

Add checks in the `rewardsSchedule` and in nested functions when working with indexes

endIndex is required to be not smaller than startIndex

add `MANAGER_ROLE` instead of manager management in treasury

transfer proxy admin ownership `hre` script

Add whitelist/propose stream script into hardhat tasks

update the validate schedule modifier

https://github.com/aurora-is-near/aurora-jet-contracts/blob/c7d92b38106dee4762a4e8102500ea1c1041a7d9/contracts/JetStakingV1.sol#L111

Can you share difference in benchmark with refunding vs without refunding ?

Not addressing this will mean that most likely any off-chain tool requires to be aware of this really deep details! I understand the main reason not to do it are gas limitations.

Can you share difference in benchmark with refunding vs without refunding.

Originally posted by @mfornet in #90 (comment)

Make sure that all private functions are internal

implement the weighting function

Allow stream manager role to update the `tau` value.

AdminControlled issues

Initiailize

• AdminControlled initializer doesn't set admin field.
• AdminControlled initializer (__AdminControlled_init) should be declared internal and without initializer modifier.

transferOwnership

To prevent the significant impact of the previous admin, it is better to revoke all actual roles on ownership transfer. Checks in transferOwnership can be bypassed with the functions in AccessControlUpgradeable.

The function transferOwnership tries to make sure that there is only one user with the admin role, and its address is also stored in the admin field. However, this is trivially bypassed using the function grantRole in AccessControlUpgradeable.

costly operations in a loop

• totalStreamShares += weightedAmountOfSharesPerStream (contracts/JetStakingV1.sol#1042)
• totalAmountOfStakedAurora += amount (contracts/JetStakingV1.sol#1034)
• totalAuroraShares += _amountOfShares (contracts/JetStakingV1.sol#1033)

Add pause script for PAUSE_ROLE

Separate pause flags for each method

adding a script for updating the storage slot(s) by the default amdin

enhance the automated static analysis process

So far the process has to be checked manually due to the false positive issues. This need to be enhanced by annotating the false positive and allow avoiding them in CI.

Add unit test where treausry`admin` can withdraw funds from the treasury rewards using `delegateCall`.

`maxDepositAmount` is an explicit duplicate for `scheduleRewards[0]`

By @paouvrard

I think maxDepositAmount can be removed as it is redundant with scheduleRewards[0], or equality must be checked if we want to make things more explicit.

update the stream lifecycle logic

Update the stream life cycle to support the following features:

• Allow multiple streams for the same token address.
• Propose/whitelist stream by only admin then deploy stream
• Creating the stream only by the stream owner
• remove stream --> move unclaimed and rest of Aurora to the stream owner.

On `rewardsSchedule` we can use memory to store `Schedule`

in this function rewardsSchedule we can use memory to store Schedule as it does not update the storage. This is optional and if done, there should there should be test cases for max array values stored in the struct. Recommending it as this function is used in other external functions and would reduce gas cost if memory variable is used.

Originally posted by @UrAvgDeveloper in #173 (comment)

Add hardhat task for `grantRole` to multiple address for the same role

LGTM with minor comments. For the CLAIM_ROLE and AIRDROP_ROLE, we will need several keys to have these roles (10 ?) so that the airdrop script can process requests in parallel. Should we use _setRoleAdmin to setup these keys separately or also grant these roles in the deployment ?

We can call grantRole() separately to add as many keys as we want. I will prepare that in a separate PR for the hardhat tasks.

Originally posted by @0x3bfc in #89 (comment)

Enhance schedule check in `_validateStreamParameters`

Schedule of the stream must be a descending function, and this should be checked.
Though this issue is not that big, since the streams are proposed by DEFAULT_ADMIN only.

Should we have a lower limit for the reward token deposit?

Good point, like a percentage of the maxDepositAmount ?

Originally posted by @paouvrard in #90 (comment)

Make function `cancelStreamProposal` callable only for proposed but not active streams

Currently, cancelStreamProposal can be called for active streams and this would result in payment from the staking contract AURORA tokens.