The Graph added a feature to send indexer rewards to a destination address, giving them more flexibility and avoid using the same address to stake and accumulate rewards.
To add this feature, the Staking contract has been refactored along with some changes in the related interface contracts.
Overall, we are happy with the changes, the code is well commented and modular functions have been added to improve readability.
Changes have been applied in PR#450 which is not merged yet.
The code has been audited by two auditors during the course of two days.
Here we present our findings:
In order to set a destination address for indexers rewards, the setRewardsDestination function has been added in the Staking contract.
Even if there's consistency with other setter functions, there's an intentional lack of validation of the _destination address parameters.
This is given by the fact that a _destination = address(0) is considered to be meaningful in the sense that in this case, the contract will re-stake those indexer rewards.
Using default values to actually have a meaningful value in the contract is discouraged, since default values are often used by clients whenever no proper value is provided.
To avoid this confusion, consider implementing a bool _restake parameter as a second input parameter and accept an empty address only if _restake == true, while performing a proper input validation if this is not the case.
One of the changes introduced was about splitting the IStaking interface to have exclusively enum and function definitions, while structs have been moved to the IStakingData interface, from which the IStaking extends.
However, the CloseAllocationRequest struct is still defined in the IStaking contract.
Consider moving the CloseAllocationRequest struct to the IStakingData to improve readability and consistency.
In the IStakingData interface, there's the use of the ABIEncoderV2 pragma clause but this is not needed at all by the interface contract.
Consider removing the ABIEncoderV2 clause from the interface file.
To execute the proposed upgrade, the Staking contract has been modified to extend from the modified IStaking interface and from the newly StakingStorageV2 contract, that exclusively adds the rewardsDestination mapping.
Conversely the old Staking contract has been renamed to StakingV1 and so the old IStaking interface, which is now IStakingV1.
To avoid confusion about which is the contract with the latest changes, and to be consistent with the nomenclature used for the storage contracts, consider renaming the latest implementation contract to StakingV2. Otherwise, consider properly commenting or adding some notes so that users can easily understand the differences between the two versions of the contracts.