staking.md

Report

Gas Optimizations

	Issue	Instances
GAS-1	a = a + b is more gas effective than a += b for state variables (excluding arrays and mappings)	3
GAS-2	Cache array length outside of loop	1
GAS-3	For Operations that will not overflow, you could use unchecked	32
GAS-4	Use Custom Errors instead of Revert Strings to save Gas	4
GAS-5	Avoid contract existence checks by using low level calls	2
GAS-6	State variables only set in the constructor should be declared immutable	1
GAS-7	Functions guaranteed to revert when called by normal users can be marked payable	1
GAS-8	++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)	3
GAS-9	Increments/decrements can be unchecked in for-loops	3
GAS-10	Use != 0 instead of > 0 for unsigned integer comparison	5

[GAS-1] a = a + b is more gas effective than a += b for state variables (excluding arrays and mappings)

This saves 16 gas per instance.

Instances (3):

File: Staking.sol

99:         totalStaked += _amount;

147:                 totalAmount += stakes[msg.sender][i].amount;

172:                 totalYield += yield;

[GAS-2] Cache array length outside of loop

If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).

Instances (1):

File: Staking.sol

224:         for (uint256 i = 0; i < userStakes.length; i++) {

[GAS-3] For Operations that will not overflow, you could use unchecked

Instances (32):

File: Staking.sol

4: import "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";

5: import "lib/openzeppelin-contracts/contracts/access/Ownable.sol";

8:     IERC20 public stakingToken; // Token used for staking

10:     uint256 public THREE_MONTHS = 90 days; // 3 months duration

11:     uint256 public SIX_MONTHS = 180 days; // 6 months duration

12:     uint256 public TWELVE_MONTHS = 365 days; // 12 months duration

22:     uint256 public THREE_MONTHS_APR = 20000; // 200% APR for 3 months

23:     uint256 public SIX_MONTHS_APR = 25000; // 250% APR for 6 months

24:     uint256 public TWELVE_MONTHS_APR = 35000; // 350% APR for 12 months

26:     uint256 public totalStaked; // Total amount of tokens staked

34:         uint256 amount; // Amount of tokens staked

35:         uint256 startTime; // Timestamp of when the stake started

36:         uint256 endTime; // Timestamp of when the stake ends

37:         uint256 apr; // APR for the stake

38:         bool claimed; // Flag to indicate if the stake has been claimed

41:     mapping(address => Stake[]) public stakes; // Mapping of user addresses to their stakes

53:         THREE_MONTHS = _threeMonths; // pass the value in seconds eg for 90 days = 7776000 sec

93:                 block.timestamp + _duration,

99:         totalStaked += _amount;

100:         emit Staked(msg.sender, _amount, _duration, apr); // Emit Staked event

120:             userStake.endTime - userStake.startTime

122:         stakingToken.transfer(msg.sender, amount + yield);

123:         totalStaked -= amount;

124:         emit Unstaked(msg.sender, amount, yield); // Emit Unstaked event

133:         return (_amount * _apr * _duration ) / (10000 * 365 days);

141:         for (uint256 i; i < userLength; i++) {

147:                 totalAmount += stakes[msg.sender][i].amount;

159:         for (uint256 i; i < userLength; i++) {

168:                     stakes[msg.sender][i].endTime -

172:                 totalYield += yield;

224:         for (uint256 i = 0; i < userStakes.length; i++) {

236:             _amount <= contractBalance - totalStaked,

[GAS-4] Use Custom Errors instead of Revert Strings to save Gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Additionally, custom errors can be used inside and outside of contracts (including interfaces and libraries).

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Consider replacing all revert strings with custom errors in the solution, and particularly those that have multiple occurrences:

Instances (4):

File: Staking.sol

71:         require(_amount > 0, "Amount must be greater than zero");

107:         require(userStake.amount > 0, "Stake does not exist");

108:         require(!userStake.claimed, "Stake already claimed");

234:         require(_amount > 0, "Amount must be greater than zero");

[GAS-5] Avoid contract existence checks by using low level calls

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence

Instances (2):

File: Staking.sol

213:         return stakingToken.balanceOf(address(this));

233:         uint256 contractBalance = stakingToken.balanceOf(address(this));

[GAS-6] State variables only set in the constructor should be declared immutable

Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)

Instances (1):

File: Staking.sol

44:         stakingToken = _stakingToken;

[GAS-7] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

Instances (1):

File: Staking.sol

232:     function withdrawExtraTokens(uint256 _amount) external onlyOwner {

[GAS-8] ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

Saves 5 gas per instance

Instances (3):

File: Staking.sol

141:         for (uint256 i; i < userLength; i++) {

159:         for (uint256 i; i < userLength; i++) {

224:         for (uint256 i = 0; i < userStakes.length; i++) {

[GAS-9] Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

These save around 25 gas saved per instance.

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existent for uint256.

Instances (3):

File: Staking.sol

141:         for (uint256 i; i < userLength; i++) {

159:         for (uint256 i; i < userLength; i++) {

224:         for (uint256 i = 0; i < userStakes.length; i++) {

[GAS-10] Use != 0 instead of > 0 for unsigned integer comparison

Instances (5):

File: Staking.sol

71:         require(_amount > 0, "Amount must be greater than zero");

107:         require(userStake.amount > 0, "Stake does not exist");

143:                 stakes[msg.sender][i].amount > 0 &&

161:                 stakes[msg.sender][i].amount > 0 &&

234:         require(_amount > 0, "Amount must be greater than zero");

Non Critical Issues

	Issue	Instances
NC-1	constants should be defined rather than using magic numbers	7
NC-2	Control structures do not follow the Solidity Style Guide	5
NC-3	Consider disabling renounceOwnership()	1
NC-4	Duplicated require()/revert() Checks Should Be Refactored To A Modifier Or Function	2
NC-5	Function ordering does not follow the Solidity style guide	1
NC-6	Functions should not be longer than 50 lines	7
NC-7	NatSpec is completely non-existent on functions that should have them	5
NC-8	Consider using named mappings	1
NC-9	Contract does not follow the Solidity style guide's suggested layout ordering	1
NC-10	Use Underscores for Number Literals (add an underscore every 3 digits)	4
NC-11	Internal and private variables and functions names should begin with an underscore	1
NC-12	Event is missing indexed fields	2
NC-13	Variables need not be initialized to zero	1

[NC-1] constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

Instances (7):

File: Staking.sol

10:     uint256 public THREE_MONTHS = 90 days; // 3 months duration

11:     uint256 public SIX_MONTHS = 180 days; // 6 months duration

12:     uint256 public TWELVE_MONTHS = 365 days; // 12 months duration

22:     uint256 public THREE_MONTHS_APR = 20000; // 200% APR for 3 months

23:     uint256 public SIX_MONTHS_APR = 25000; // 250% APR for 6 months

24:     uint256 public TWELVE_MONTHS_APR = 35000; // 350% APR for 12 months

133:         return (_amount * _apr * _duration ) / (10000 * 365 days);

[NC-2] Control structures do not follow the Solidity Style Guide

See the control structures section of the Solidity Style Guide

Instances (5):

File: Staking.sol

38:         bool claimed; // Flag to indicate if the stake has been claimed

48:     function modifyTimeFrames(

59:     function modifyAPRs(

142:             if (

160:             if (

[NC-3] Consider disabling renounceOwnership()

If the plan for your project does not include eventually giving up all ownership control, consider overwriting OpenZeppelin's Ownable's renounceOwnership() function in order to disable it.

Instances (1):

File: Staking.sol

7: contract StakingContract is Ownable(msg.sender) {

[NC-4] Duplicated require()/revert() Checks Should Be Refactored To A Modifier Or Function

Instances (2):

File: Staking.sol

71:         require(_amount > 0, "Amount must be greater than zero");

234:         require(_amount > 0, "Amount must be greater than zero");

[NC-5] Function ordering does not follow the Solidity style guide

According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern

Instances (1):

File: Staking.sol

1: 
   Current order:
   external modifyTimeFrames
   external modifyAPRs
   external stake
   external unstake
   internal calculateYield
   external getMatureStakes
   external getMatureYield
   external getTotalStaked
   external getStakeCount
   external getStake
   external getContractTokenBalance
   external getUserStakeAmounts
   external withdrawExtraTokens
   
   Suggested order:
   external modifyTimeFrames
   external modifyAPRs
   external stake
   external unstake
   external getMatureStakes
   external getMatureYield
   external getTotalStaked
   external getStakeCount
   external getStake
   external getContractTokenBalance
   external getUserStakeAmounts
   external withdrawExtraTokens
   internal calculateYield

[NC-6] Functions should not be longer than 50 lines

Overly complex code can make understanding functionality more difficult, try to further modularize your code to ensure readability

Instances (7):

File: Staking.sol

70:     function stake(uint256 _amount, uint256 _duration) external {

136:     function getMatureStakes() external view returns (uint256) {

154:     function getMatureYield() external view returns (uint256) {

180:     function getTotalStaked() external view returns (uint256) {

185:     function getStakeCount(address _user) external view returns (uint256) {

212:     function getContractTokenBalance() external view returns (uint256) {

232:     function withdrawExtraTokens(uint256 _amount) external onlyOwner {

[NC-7] NatSpec is completely non-existent on functions that should have them

Public and external functions that aren't view or pure should have NatSpec comments

Instances (5):

File: Staking.sol

48:     function modifyTimeFrames(

59:     function modifyAPRs(

70:     function stake(uint256 _amount, uint256 _duration) external {

105:     function unstake(uint256 _index) external {

232:     function withdrawExtraTokens(uint256 _amount) external onlyOwner {

[NC-8] Consider using named mappings

Consider moving to solidity version 0.8.18 or later, and using named mappings to make it easier to understand the purpose of each mapping

Instances (1):

File: Staking.sol

41:     mapping(address => Stake[]) public stakes; // Mapping of user addresses to their stakes

[NC-9] Contract does not follow the Solidity style guide's suggested layout ordering

The style guide says that, within a contract, the ordering should be:

1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

However, the contract(s) below do not follow this ordering

Instances (1):

File: Staking.sol

1: 
   Current order:
   VariableDeclaration.stakingToken
   VariableDeclaration.THREE_MONTHS
   VariableDeclaration.SIX_MONTHS
   VariableDeclaration.TWELVE_MONTHS
   VariableDeclaration.THREE_MONTHS_APR
   VariableDeclaration.SIX_MONTHS_APR
   VariableDeclaration.TWELVE_MONTHS_APR
   VariableDeclaration.totalStaked
   EventDefinition.Staked
   EventDefinition.Unstaked
   StructDefinition.Stake
   VariableDeclaration.stakes
   FunctionDefinition.constructor
   FunctionDefinition.modifyTimeFrames
   FunctionDefinition.modifyAPRs
   FunctionDefinition.stake
   FunctionDefinition.unstake
   FunctionDefinition.calculateYield
   FunctionDefinition.getMatureStakes
   FunctionDefinition.getMatureYield
   FunctionDefinition.getTotalStaked
   FunctionDefinition.getStakeCount
   FunctionDefinition.getStake
   FunctionDefinition.getContractTokenBalance
   FunctionDefinition.getUserStakeAmounts
   FunctionDefinition.withdrawExtraTokens
   
   Suggested order:
   VariableDeclaration.stakingToken
   VariableDeclaration.THREE_MONTHS
   VariableDeclaration.SIX_MONTHS
   VariableDeclaration.TWELVE_MONTHS
   VariableDeclaration.THREE_MONTHS_APR
   VariableDeclaration.SIX_MONTHS_APR
   VariableDeclaration.TWELVE_MONTHS_APR
   VariableDeclaration.totalStaked
   VariableDeclaration.stakes
   StructDefinition.Stake
   EventDefinition.Staked
   EventDefinition.Unstaked
   FunctionDefinition.constructor
   FunctionDefinition.modifyTimeFrames
   FunctionDefinition.modifyAPRs
   FunctionDefinition.stake
   FunctionDefinition.unstake
   FunctionDefinition.calculateYield
   FunctionDefinition.getMatureStakes
   FunctionDefinition.getMatureYield
   FunctionDefinition.getTotalStaked
   FunctionDefinition.getStakeCount
   FunctionDefinition.getStake
   FunctionDefinition.getContractTokenBalance
   FunctionDefinition.getUserStakeAmounts
   FunctionDefinition.withdrawExtraTokens

[NC-10] Use Underscores for Number Literals (add an underscore every 3 digits)

Instances (4):

File: Staking.sol

22:     uint256 public THREE_MONTHS_APR = 20000; // 200% APR for 3 months

23:     uint256 public SIX_MONTHS_APR = 25000; // 250% APR for 6 months

24:     uint256 public TWELVE_MONTHS_APR = 35000; // 350% APR for 12 months

53:         THREE_MONTHS = _threeMonths; // pass the value in seconds eg for 90 days = 7776000 sec

[NC-11] Internal and private variables and functions names should begin with an underscore

According to the Solidity Style Guide, Non-external variable and function names should begin with an underscore

Instances (1):

File: Staking.sol

128:     function calculateYield(

[NC-12] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Instances (2):

File: Staking.sol

29:     event Staked(address indexed user, uint256 amount, uint256 duration, uint256 apr);

30:     event Unstaked(address indexed user, uint256 amount, uint256 yield);

[NC-13] Variables need not be initialized to zero

The default value for variables is zero, so initializing them to zero is superfluous.

Instances (1):

File: Staking.sol

224:         for (uint256 i = 0; i < userStakes.length; i++) {

Low Issues

	Issue	Instances
L-1	Use a 2-step ownership transfer pattern	1
L-2	Some tokens may revert when zero value transfers are made	3
L-3	Division by zero not prevented	1
L-4	Solidity version 0.8.20+ may not work on other chains due to PUSH0	1
L-5	Use Ownable2Step.transferOwnership instead of Ownable.transferOwnership	1
L-6	File allows a version of solidity that is susceptible to an assembly optimizer bug	1
L-7	Unsafe ERC20 operation(s)	3

[L-1] Use a 2-step ownership transfer pattern

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account. Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

Instances (1):

File: Staking.sol

7: contract StakingContract is Ownable(msg.sender) {

[L-2] Some tokens may revert when zero value transfers are made

Example: https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers.

In spite of the fact that EIP-20 states that zero-valued transfers must be accepted, some tokens, such as LEND will revert if this is attempted, which may cause transactions that involve other tokens (such as batch operations) to fully revert. Consider skipping the transfer if the amount is zero, which will also save gas.

Instances (3):

File: Staking.sol

88:         stakingToken.transferFrom(msg.sender, address(this), _amount);

122:         stakingToken.transfer(msg.sender, amount + yield);

240:         stakingToken.transfer(msg.sender, _amount);

[L-3] Division by zero not prevented

The divisions below take an input parameter which does not have any zero-value checks, which may lead to the functions reverting when zero is passed.

Instances (1):

File: Staking.sol

133:         return (_amount * _apr * _duration ) / (10000 * 365 days);

[L-4] Solidity version 0.8.20+ may not work on other chains due to PUSH0

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which includes the new PUSH0 op code. This op code may not yet be implemented on all L2s, so deployment on these chains will fail. To work around this issue, use an earlier EVM version. While the project itself may or may not compile with 0.8.20, other projects with which it integrates, or which extend this project may, and those projects will have problems deploying these contracts/libraries.

Instances (1):

File: Staking.sol

2: pragma solidity ^0.8.0;

[L-5] Use Ownable2Step.transferOwnership instead of Ownable.transferOwnership

Use Ownable2Step.transferOwnership which is safer. Use it as it is more secure due to 2-stage ownership transfer.

Recommended Mitigation Steps

Use Ownable2Step.sol

    function acceptOwnership() external {
        address sender = _msgSender();
        require(pendingOwner() == sender, "Ownable2Step: caller is not the new owner");
        _transferOwnership(sender);
    }

Instances (1):

File: Staking.sol

5: import "lib/openzeppelin-contracts/contracts/access/Ownable.sol";

[L-6] File allows a version of solidity that is susceptible to an assembly optimizer bug

In solidity versions 0.8.13 and 0.8.14, there is an optimizer bug where, if the use of a variable is in a separate assembly block from the block in which it was stored, the mstore operation is optimized out, leading to uninitialized memory. The code currently does not have such a pattern of execution, but it does use mstores in assembly blocks, so it is a risk for future changes. The affected solidity versions should be avoided if at all possible.

Instances (1):

File: Staking.sol

2: pragma solidity ^0.8.0;

[L-7] Unsafe ERC20 operation(s)

Instances (3):

File: Staking.sol

88:         stakingToken.transferFrom(msg.sender, address(this), _amount);

122:         stakingToken.transfer(msg.sender, amount + yield);

240:         stakingToken.transfer(msg.sender, _amount);

Medium Issues

	Issue	Instances
M-1	Contracts are vulnerable to fee-on-transfer accounting-related issues	1
M-2	Centralization Risk for trusted owners	4
M-3	Return values of transfer()/transferFrom() not checked	3
M-4	Unsafe use of transfer()/transferFrom() with IERC20	3

[M-1] Contracts are vulnerable to fee-on-transfer accounting-related issues

Consistently check account balance before and after transfers for Fee-On-Transfer discrepancies. As arbitrary ERC20 tokens can be used, the amount here should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter. Or explicitly document that such tokens shouldn't be used and won't be supported

Instances (1):

File: Staking.sol

88:         stakingToken.transferFrom(msg.sender, address(this), _amount);

[M-2] Centralization Risk for trusted owners

Impact:

Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds.

Instances (4):

File: Staking.sol

7: contract StakingContract is Ownable(msg.sender) {

52:     ) external onlyOwner {

63:     ) external onlyOwner {

232:     function withdrawExtraTokens(uint256 _amount) external onlyOwner {

[M-3] Return values of transfer()/transferFrom() not checked

Not all IERC20 implementations revert() when there's a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment

Instances (3):

File: Staking.sol

88:         stakingToken.transferFrom(msg.sender, address(this), _amount);

122:         stakingToken.transfer(msg.sender, amount + yield);

240:         stakingToken.transfer(msg.sender, _amount);

[M-4] Unsafe use of transfer()/transferFrom() with IERC20

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions on L1 do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert (see this link for a test case). Use OpenZeppelin's SafeERC20's safeTransfer()/safeTransferFrom() instead

Instances (3):

File: Staking.sol

88:         stakingToken.transferFrom(msg.sender, address(this), _amount);

122:         stakingToken.transfer(msg.sender, amount + yield);

240:         stakingToken.transfer(msg.sender, _amount);