| Audit Date | 29/04/2023 - 03/05/2023 |
| Auditor | HollaDieWaldfee (@HollaWaldfee100) |
| Version 1 | 03/05/2023 Initial Report |
The following smart contract audit report is based on the information and code provided by the client, and any findings or recommendations are made solely on the basis of this information. While the Auditor has exercised due care and skill in conducting the audit, it cannot be guaranteed that all issues have been identified and that there are no undiscovered errors or vulnerabilities in the code.
Furthermore, this report is not an endorsement or certification of the smart contract, and the Auditor does not assume any responsibility for any losses or damages that may result from the use of the smart contracts, either in their current form or in any modified version thereof.
HollaDieWaldfee is a top ranked Smart Contract Auditor doing audits on code4rena (www.code4rena.com) and Sherlock (www.sherlock.xyz), having ranked 1st in multiple contests.
On Sherlock he uses the handle "roguereddwarf" to compete in contests.
He can also be booked for conducting Private Audits.
Contact:
Twitter: @HollaWaldfee100
The audit has been conducted in the "key-for-gmx" repository which is private:
The commit hash at the start of the audit was:
ca1fd296812fec7048ac8b573c92a1180c328446
During the duration of the audit, the client opened a public repository (https://github.com/KeyFinanceTeam/key-finance-contracts) and new commits have been made such that the final commit was:
60cb18161e53dce1de692881cb8ae974855ecc42
Files included in the audit:
- /contracts/Staker.sol
- /contracts/Rewards.sol
- /contracts/TransferReceiver.sol
- /contracts/Converter.sol
- /contracts/FeeCalculator.sol
| Severity | Impact: High | Impact: Medium | Impact: Low |
|---|---|---|---|
| Likelihood: High |  |
|
 |
| Likelihood: Medium | |
|
 |
| Likelihood: Low | |
|
|
Impact - the technical, economic and reputation damage of a successful attack
Likelihood - the chance that a particular vulnerability is discovered and exploited
| Severity | Total | Fixed | Acknowledged | Disputed | Reported |
|---|---|---|---|---|---|
|
0 | 0 | 0 | 0 | 0 |
|
1 | 0 | 1 | 0 | 0 |
|
4 | 4 | 0 | 0 | 0 |
|
1 | 1 | 0 | 0 | 0 |
The protocol makes use of trusted roles (admin + operator) that are able to set fee percentages, pause reward updating and reward claiming.
Also the TransferReceiver is upgradeable with a time delay of at least 2 days. It means that the admin is fully trusted to handle user funds. However, users will also be able to reclaim their accounts through a buyback feature that will be developed in the future. The client acknowledged this is necessary because changes will need to be adapted as GMX upgrades occur in the future.
Description:
All parameters in the Rewards.sol constructor are checked to not be equal to address(0) except for _GMXkey.
contracts/Rewards.sol#L65-L89
constructor(address _admin, IRewardRouter _rewardRouter, address _GMXkey, address _esGMXkey, address _MPkey, address _staker, address _treasury) Pausable(_admin) {
require(address(_rewardRouter) != address(0), "Rewards: rewardRouter must not be zero address");
require(_esGMXkey != address(0), "Rewards: esGMXkey must not be zero address");
require(_MPkey != address(0), "Rewards: MPkey must not be zero address");
require(_staker != address(0), "Rewards: staker must not be zero address");
require(_treasury != address(0), "Rewards: treasury must not be zero address");
stakedGmxTracker = _rewardRouter.stakedGmxTracker();
require(stakedGmxTracker != address(0), "Rewards: stakedGmxTracker must not be zero address");
feeGmxTracker = _rewardRouter.feeGmxTracker();
require(feeGmxTracker != address(0), "Rewards: feeGmxTracker must not be zero address");
gmx = _rewardRouter.gmx();
require(gmx != address(0), "Rewards: gmx must not be zero address");
esGmx = _rewardRouter.esGmx();
require(esGmx != address(0), "Rewards: esGmx must not be zero address");
bnGmx = _rewardRouter.bnGmx();
require(bnGmx != address(0), "Rewards: bnGmx must not be zero address");
GMXkey = _GMXkey;
esGMXkey = _esGMXkey;
MPkey = _MPkey;
weth = _rewardRouter.weth();
require(weth != address(0), "Rewards: weth must not be zero address");
staker = _staker;
treasury = _treasury;
maxPeriodsToUpdateRewards = 4;
}Recommendation:
Add a zero address check for _GMXkey.
Fix:
The zero address check has been added during the audit in the following commit: 28dcee08877a7d8334aa1f97d4ebccf8e803df7a.
3. Wrong accounting of accumulated rewards if rewards from TransferReceiver are not updated in each period 
Description:
When the Rewards.updateAllRewardsForTransferReceiverAndTransferFee function is called for a receiver and a new period is entered, the _initializePeriod function is called which applies the cumulated rewards which is performed by the _applyCumulatedReward function:
contracts/Rewards.sol#L464-L467
function _applyCumulatedReward(address stakingToken, address rewardToken, uint256 totalShareForPrevPeriod) internal {
rewardPerUnit[stakingToken][rewardToken][currentPeriodIndex] += Math.mulDiv(cumulatedReward[stakingToken][rewardToken], PRECISION, totalShareForPrevPeriod);
cumulatedReward[stakingToken][rewardToken] = 0;
}totalShareForPrevPeriod is the total shares for the previous period for a given staked token (GMXkey, esGMXkey, MPkey).
The rewards should thus be added to the previous period. However the previous period is not necessarily equal to currentPeriodIndex if updating rewards for the receiver is skipped for one or more periods.
Thus the accumulated rewards would be added to the wrong period.
Recommendation:
The accumulated rewards need to be added to the previous period which can be calculated by block.timestamp / PERIOD - 1.
Fix:
The issue has been fixed during the audit in the following commit: 79f1f635ffb5c164019078b70f35b8719f44a432.
The fix implements the recommendation and the accumulated rewards are now added to the previous period, not to the period when rewards were last updated.
Description:
The TransferReceiver contract inherits from ReentrancyGuard.
contracts/TransferReceiver.sol#L31
contract TransferReceiver is ITransferReceiver, Initializable, UUPSUpgradeable, ConfigUserInitializable, ReentrancyGuard, PausableInitializable {The problem with this is that TransferReceiver is supposed to be an upgradeable contract and is therefore initialized by an initializer function.
However the initializer function does not initialize the _status variable of the ReentrancyGuard.
Therefore the _status variable starts with its default value which is 0. This is an undefined state as states are only defined for the values 1 and 2:
ReentrancyGuard.sol#L34-L35
uint256 private constant _NOT_ENTERED = 1;
uint256 private constant _ENTERED = 2;Upon first use of the ReentrancyGuard though, the _status is then updated to a defined value and the issue has resolved itself:
ReentrancyGuard.sol#L56-L62
function _nonReentrantBefore() private {
// On the first call to nonReentrant, _status will be _NOT_ENTERED
require(_status != _ENTERED, "ReentrancyGuard: reentrant call");
// Any calls to nonReentrant after this point will fail
_status = _ENTERED;
}An additional risk is the lack of a storage gap in the ReentrancyGuard contract which can lead to storage collisions (also see issue #5 regarding this).
Recommendation:
It is recommended that ReentrancyGuardUpgradeable is used (https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/security/ReentrancyGuardUpgradeable.sol) which exposes the __ReentrancyGuard_init function. This function must then be called in TransferReceiver.initialize.
Fix:
The issue has been fixed during the audit in the following commit: 3689490e39d31eff766bb0815ae621efec7c41d4.
The fix implements the recommendation.
Description:
TransferReceiver is an upgradeable contract and inherits from multiple parent contracts.
contracts/TransferReceiver.sol#L31
contract TransferReceiver is ITransferReceiver, Initializable, UUPSUpgradeable, ConfigUserInitializable, ReentrancyGuardUpgradeable, PausableInitializable {Three of these contracts do not have a storage gap implemented.
These contracts are ConfigUserInitializable, PausableInitializable and AdminableInitializable (a parent contract of PausableInitializable).
This means that it's not possible to add additional variables to these three contracts since it would lead to storage collisions.
Refer to the OZ documentation for more information about storage gaps:
https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#storage-gaps
Recommendation:
I recommended to implement storage gaps in the three contracts mentioned above.
diff --git a/contracts/common/AdminableInitializable.sol b/contracts/common/AdminableInitializable.sol
index c68d6bd..386349d 100644
--- a/contracts/common/AdminableInitializable.sol
+++ b/contracts/common/AdminableInitializable.sol
@@ -33,4 +33,6 @@ abstract contract AdminableInitializable {
admin = candidate;
candidate = address(0);
}
+
+ uint256[64] private __gap;
}
\ No newline at end of file
diff --git a/contracts/common/ConfigUserInitializable.sol b/contracts/common/ConfigUserInitializable.sol
index 60a4037..533a4ce 100644
--- a/contracts/common/ConfigUserInitializable.sol
+++ b/contracts/common/ConfigUserInitializable.sol
@@ -10,4 +10,6 @@ contract ConfigUserInitializable {
require(_config != address(0), "ConfigUserInitializable: config is the zero address");
config = _config;
}
+
+ uint256[64] private __gap;
}
\ No newline at end of file
diff --git a/contracts/common/PausableInitializable.sol b/contracts/common/PausableInitializable.sol
index 1cae26a..8263604 100644
--- a/contracts/common/PausableInitializable.sol
+++ b/contracts/common/PausableInitializable.sol
@@ -29,4 +29,6 @@ abstract contract PausableInitializable is AdminableInitializable {
paused = false;
emit Resumed();
}
+
+ uint256[64] private __gap;
}Fix:
The client fixed the issue during the audit in the following commit: 7d4fd9945c4840e17e7307c8ac1847966bfd9225.
Description:
The Converter contract creates a new proxy for each TransferReceiver that is created.
It's unnecessary that there's also a new implementation contract created for each proxy:
contracts/Converter.sol#L157-L167
TransferReceiver transferReceiver = new TransferReceiver();
ERC1967Proxy proxy = new ERC1967Proxy(
address(transferReceiver),
abi.encodeWithSelector(ITransferReceiver(transferReceiver).initialize.selector,
operator,
config,
address(this),
rewardRouter,
stakedGlp,
rewards)
);All the proxies can use the same implementation contract.
Recommendation:
Set the TransferReceiver implementation contract in the constructor of the Converter and use it when creating new proxies.
Fix:
The sponsor implemented the recommendation during the audit in the following commit: e28a2ce779320cd37001a2d6e48416dec0e09f42.
In addition to the recommendation the TransferReceiver implementation contract can also be updated by the admin via the new setTransferReceiver function such that new proxies get created with the new implementation.