feat(basefee-treasury): implement `impl` #142

github-actions · 2024-11-13T10:59:08Z

Description

PR to merge from implement-feature/basefee-treasury/impl to feature/basefee-treasury.

github-actions · 2024-11-14T09:10:20Z

Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

reentrancy-eth (6 results) (High)
shadowing-state (4 results) (High)
uninitialized-state (13 results) (High)

reentrancy-eth

Impact: High
Confidence: Medium

ID-0
Reentrancy in CoinbaseExecution.wrapUpEpoch():
External calls:

dpos-contract/src/ronin/validator/CoinbaseExecution.sol

Lines 102 to 162 in 0b6d306

    
           function wrapUpEpoch() external payable virtual override onlyCoinbase whenEpochEnding oncePerEpoch { 
        
             unchecked { 
        
               uint256 newPeriod = _computePeriod(block.timestamp); 
        
               bool periodEnding = _isPeriodEnding(newPeriod); 
        
               uint256 lastPeriod = currentPeriod(); 
        
               uint256 epoch = epochOf(block.number); 
        
               uint256 nextEpoch = epoch + 1; 
        
               IRandomBeacon randomBeacon = IRandomBeacon(getContract(ContractType.RANDOM_BEACON)); 
        
               // This request is actually only invoked at the first epoch of the period. 
        
               randomBeacon.execRequestRandomSeedForNextPeriod(lastPeriod, newPeriod); 
        
               // Get all candidate ids 
        
               address[] memory allCids = _candidateIds; 
        
               _syncFastFinalityReward({ epoch: epoch, validatorIds: allCids }); 
        
               if (periodEnding) { 
        
                 ISlashIndicator slashIndicatorContract = ISlashIndicator(getContract(ContractType.SLASH_INDICATOR)); 
        
                 // Slash submit random beacon proof unavailability first, then update credit scores. 
        
                 randomBeacon.execRecordAndSlashUnavailability(lastPeriod, newPeriod, address(slashIndicatorContract), allCids); 
        
                 slashIndicatorContract.execUpdateCreditScores(allCids, lastPeriod); 
        
                 (uint256[] memory delegatorBlockMiningRewards, uint256[] memory delegatorFastFinalityRewards) = 
        
                   _distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(lastPeriod, allCids); 
        
                 _settleAndTransferDelegatingRewards( 
        
                   lastPeriod, allCids, delegatorBlockMiningRewards, delegatorFastFinalityRewards 
        
                 ); 
        
                 _tryRecycleLockedFundsFromEmergencyExits(); 
        
                 _recycleDeprecatedRewards(); 
        
                 address[] memory revokedCandidateIds = _syncCandidateSet(newPeriod); 
        
                 if (revokedCandidateIds.length > 0) { 
        
                   // Re-update `allCids` after unsatisfied candidates get removed. 
        
                   allCids = _candidateIds; 
        
                   slashIndicatorContract.execResetCreditScores(revokedCandidateIds); 
        
                 } 
        
                 // Wrap up the beacon period includes (1) finalizing the beacon proof, and (2) determining the validator list for the next period by new proof. 
        
                 // Should wrap up the beacon after unsatisfied candidates get removed. 
        
                 randomBeacon.execFinalizeBeaconAndPendingCids(lastPeriod, newPeriod, allCids); 
        
                 _periodEndBlock[lastPeriod] = block.number; 
        
                 _currentPeriodStartAtBlock = block.number + 1; 
        
               } 
        
               // Clear the previous validator set and block producer set before sync the new set from beacon. 
        
               _clearPreviousValidatorSetAndBlockProducerSet(); 
        
               // Query the new validator set for upcoming epoch from the random beacon contract. 
        
               // Save new set into the contract storage. 
        
               address[] memory newValidatorIds = _syncValidatorSet(randomBeacon, newPeriod, nextEpoch); 
        
               // Activate applicable validators into the block producer set. 
        
               _updateApplicableValidatorToBlockProducerSet(newPeriod, nextEpoch, newValidatorIds); 
        
               emit WrappedUpEpoch(lastPeriod, epoch, periodEnding); 
        
               _periodOf[nextEpoch] = newPeriod; 
        
               _lastUpdatedPeriod = newPeriod; 
        
             } 
        
           }

ID-1
Reentrancy in CoinbaseExecution._distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(uint256,address[]):
External calls:
- _distributeMiningReward(vId,treasury)
  - (success,None) = recipient.call{gas: gas,value: amount}()
- _distributeFastFinalityReward(vId,treasury,validatorFFReward)
  - (success,None) = recipient.call{gas: gas,value: amount}()
    State variables written after the call(s):
- delete _delegatorMiningReward[vId]
  CommonStorage._delegatorMiningReward can be used in cross function reentrancies:
- CoinbaseExecution._distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(uint256,address[])
- CoinbaseExecution.submitBlockReward()
- delete _fastFinalityReward[vId]
  CommonStorage._fastFinalityReward can be used in cross function reentrancies:
- CoinbaseExecution._distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(uint256,address[])
- CoinbaseExecution._syncFastFinalityReward(uint256,address[])
- _totalDeprecatedReward += _validatorMiningReward[vId] + _delegatorMiningReward[vId] + _fastFinalityReward[vId]
  CommonStorage._totalDeprecatedReward can be used in cross function reentrancies:
- CoinbaseExecution._distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(uint256,address[])
- CoinbaseExecution._recycleDeprecatedRewards()
- CoinbaseExecution._syncFastFinalityReward(uint256,address[])
- EmergencyExit._tryRecycleLockedFundsFromEmergencyExits()
- CoinbaseExecution.submitBlockReward()
- CommonStorage.totalDeprecatedReward()
- delete _validatorMiningReward[vId]
  CommonStorage._validatorMiningReward can be used in cross function reentrancies:
- CoinbaseExecution._distributeMiningReward(address,address)
- CoinbaseExecution._distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(uint256,address[])
- CoinbaseExecution.submitBlockReward()

dpos-contract/src/ronin/validator/CoinbaseExecution.sol

Lines 206 to 244 in 0b6d306

    
           function _distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward( 
        
             uint256 lastPeriod, 
        
             address[] memory cids 
        
           ) private returns (uint256[] memory delegatorBlockMiningRewards, uint256[] memory delegatorFastFinalityRewards) { 
        
             address vId; // validator id 
        
             address payable treasury; 
        
             uint256 length = cids.length; 
        
             delegatorBlockMiningRewards = new uint256[](length); 
        
             delegatorFastFinalityRewards = new uint256[](length); 
        
             (uint256 minRate, uint256 maxRate) = IStaking(getContract(ContractType.STAKING)).getCommissionRateRange(); 
        
             for (uint256 i; i < length; ++i) { 
        
               vId = cids[i]; 
        
               treasury = _candidateInfo[vId].__shadowedTreasury; 
        
               if (!_isJailedById(vId) && !_miningRewardDeprecatedById(vId, lastPeriod)) { 
        
                 (uint256 validatorFFReward, uint256 delegatorFFReward) = _calcCommissionReward({ 
        
                   vId: vId, 
        
                   totalReward: _fastFinalityReward[vId], 
        
                   maxCommissionRate: maxRate, 
        
                   minCommissionRate: minRate 
        
                 }); 
        
                 delegatorBlockMiningRewards[i] = _delegatorMiningReward[vId]; 
        
                 delegatorFastFinalityRewards[i] = delegatorFFReward; 
        
                 _distributeMiningReward(vId, treasury); 
        
                 _distributeFastFinalityReward(vId, treasury, validatorFFReward); 
        
               } else { 
        
                 _totalDeprecatedReward += _validatorMiningReward[vId] + _delegatorMiningReward[vId] + _fastFinalityReward[vId]; 
        
               } 
        
               delete _delegatorMiningReward[vId]; 
        
               delete _validatorMiningReward[vId]; 
        
               delete _fastFinalityReward[vId]; 
        
             } 
        
           }

ID-2
Reentrancy in CoinbaseExecution.wrapUpEpoch():
External calls:

dpos-contract/src/ronin/validator/CoinbaseExecution.sol

Lines 102 to 162 in 0b6d306

    
           function wrapUpEpoch() external payable virtual override onlyCoinbase whenEpochEnding oncePerEpoch { 
        
             unchecked { 
        
               uint256 newPeriod = _computePeriod(block.timestamp); 
        
               bool periodEnding = _isPeriodEnding(newPeriod); 
        
               uint256 lastPeriod = currentPeriod(); 
        
               uint256 epoch = epochOf(block.number); 
        
               uint256 nextEpoch = epoch + 1; 
        
               IRandomBeacon randomBeacon = IRandomBeacon(getContract(ContractType.RANDOM_BEACON)); 
        
               // This request is actually only invoked at the first epoch of the period. 
        
               randomBeacon.execRequestRandomSeedForNextPeriod(lastPeriod, newPeriod); 
        
               // Get all candidate ids 
        
               address[] memory allCids = _candidateIds; 
        
               _syncFastFinalityReward({ epoch: epoch, validatorIds: allCids }); 
        
               if (periodEnding) { 
        
                 ISlashIndicator slashIndicatorContract = ISlashIndicator(getContract(ContractType.SLASH_INDICATOR)); 
        
                 // Slash submit random beacon proof unavailability first, then update credit scores. 
        
                 randomBeacon.execRecordAndSlashUnavailability(lastPeriod, newPeriod, address(slashIndicatorContract), allCids); 
        
                 slashIndicatorContract.execUpdateCreditScores(allCids, lastPeriod); 
        
                 (uint256[] memory delegatorBlockMiningRewards, uint256[] memory delegatorFastFinalityRewards) = 
        
                   _distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(lastPeriod, allCids); 
        
                 _settleAndTransferDelegatingRewards( 
        
                   lastPeriod, allCids, delegatorBlockMiningRewards, delegatorFastFinalityRewards 
        
                 ); 
        
                 _tryRecycleLockedFundsFromEmergencyExits(); 
        
                 _recycleDeprecatedRewards(); 
        
                 address[] memory revokedCandidateIds = _syncCandidateSet(newPeriod); 
        
                 if (revokedCandidateIds.length > 0) { 
        
                   // Re-update `allCids` after unsatisfied candidates get removed. 
        
                   allCids = _candidateIds; 
        
                   slashIndicatorContract.execResetCreditScores(revokedCandidateIds); 
        
                 } 
        
                 // Wrap up the beacon period includes (1) finalizing the beacon proof, and (2) determining the validator list for the next period by new proof. 
        
                 // Should wrap up the beacon after unsatisfied candidates get removed. 
        
                 randomBeacon.execFinalizeBeaconAndPendingCids(lastPeriod, newPeriod, allCids); 
        
                 _periodEndBlock[lastPeriod] = block.number; 
        
                 _currentPeriodStartAtBlock = block.number + 1; 
        
               } 
        
               // Clear the previous validator set and block producer set before sync the new set from beacon. 
        
               _clearPreviousValidatorSetAndBlockProducerSet(); 
        
               // Query the new validator set for upcoming epoch from the random beacon contract. 
        
               // Save new set into the contract storage. 
        
               address[] memory newValidatorIds = _syncValidatorSet(randomBeacon, newPeriod, nextEpoch); 
        
               // Activate applicable validators into the block producer set. 
        
               _updateApplicableValidatorToBlockProducerSet(newPeriod, nextEpoch, newValidatorIds); 
        
               emit WrappedUpEpoch(lastPeriod, epoch, periodEnding); 
        
               _periodOf[nextEpoch] = newPeriod; 
        
               _lastUpdatedPeriod = newPeriod; 
        
             } 
        
           }

ID-3
Reentrancy in CoinbaseExecution.wrapUpEpoch():
External calls:

dpos-contract/src/ronin/validator/CoinbaseExecution.sol

Lines 102 to 162 in 0b6d306

    
           function wrapUpEpoch() external payable virtual override onlyCoinbase whenEpochEnding oncePerEpoch { 
        
             unchecked { 
        
               uint256 newPeriod = _computePeriod(block.timestamp); 
        
               bool periodEnding = _isPeriodEnding(newPeriod); 
        
               uint256 lastPeriod = currentPeriod(); 
        
               uint256 epoch = epochOf(block.number); 
        
               uint256 nextEpoch = epoch + 1; 
        
               IRandomBeacon randomBeacon = IRandomBeacon(getContract(ContractType.RANDOM_BEACON)); 
        
               // This request is actually only invoked at the first epoch of the period. 
        
               randomBeacon.execRequestRandomSeedForNextPeriod(lastPeriod, newPeriod); 
        
               // Get all candidate ids 
        
               address[] memory allCids = _candidateIds; 
        
               _syncFastFinalityReward({ epoch: epoch, validatorIds: allCids }); 
        
               if (periodEnding) { 
        
                 ISlashIndicator slashIndicatorContract = ISlashIndicator(getContract(ContractType.SLASH_INDICATOR)); 
        
                 // Slash submit random beacon proof unavailability first, then update credit scores. 
        
                 randomBeacon.execRecordAndSlashUnavailability(lastPeriod, newPeriod, address(slashIndicatorContract), allCids); 
        
                 slashIndicatorContract.execUpdateCreditScores(allCids, lastPeriod); 
        
                 (uint256[] memory delegatorBlockMiningRewards, uint256[] memory delegatorFastFinalityRewards) = 
        
                   _distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(lastPeriod, allCids); 
        
                 _settleAndTransferDelegatingRewards( 
        
                   lastPeriod, allCids, delegatorBlockMiningRewards, delegatorFastFinalityRewards 
        
                 ); 
        
                 _tryRecycleLockedFundsFromEmergencyExits(); 
        
                 _recycleDeprecatedRewards(); 
        
                 address[] memory revokedCandidateIds = _syncCandidateSet(newPeriod); 
        
                 if (revokedCandidateIds.length > 0) { 
        
                   // Re-update `allCids` after unsatisfied candidates get removed. 
        
                   allCids = _candidateIds; 
        
                   slashIndicatorContract.execResetCreditScores(revokedCandidateIds); 
        
                 } 
        
                 // Wrap up the beacon period includes (1) finalizing the beacon proof, and (2) determining the validator list for the next period by new proof. 
        
                 // Should wrap up the beacon after unsatisfied candidates get removed. 
        
                 randomBeacon.execFinalizeBeaconAndPendingCids(lastPeriod, newPeriod, allCids); 
        
                 _periodEndBlock[lastPeriod] = block.number; 
        
                 _currentPeriodStartAtBlock = block.number + 1; 
        
               } 
        
               // Clear the previous validator set and block producer set before sync the new set from beacon. 
        
               _clearPreviousValidatorSetAndBlockProducerSet(); 
        
               // Query the new validator set for upcoming epoch from the random beacon contract. 
        
               // Save new set into the contract storage. 
        
               address[] memory newValidatorIds = _syncValidatorSet(randomBeacon, newPeriod, nextEpoch); 
        
               // Activate applicable validators into the block producer set. 
        
               _updateApplicableValidatorToBlockProducerSet(newPeriod, nextEpoch, newValidatorIds); 
        
               emit WrappedUpEpoch(lastPeriod, epoch, periodEnding); 
        
               _periodOf[nextEpoch] = newPeriod; 
        
               _lastUpdatedPeriod = newPeriod; 
        
             } 
        
           }

ID-4
Reentrancy in CoinbaseExecution.wrapUpEpoch():
External calls:

dpos-contract/src/ronin/validator/CoinbaseExecution.sol

Lines 102 to 162 in 0b6d306

    
           function wrapUpEpoch() external payable virtual override onlyCoinbase whenEpochEnding oncePerEpoch { 
        
             unchecked { 
        
               uint256 newPeriod = _computePeriod(block.timestamp); 
        
               bool periodEnding = _isPeriodEnding(newPeriod); 
        
               uint256 lastPeriod = currentPeriod(); 
        
               uint256 epoch = epochOf(block.number); 
        
               uint256 nextEpoch = epoch + 1; 
        
               IRandomBeacon randomBeacon = IRandomBeacon(getContract(ContractType.RANDOM_BEACON)); 
        
               // This request is actually only invoked at the first epoch of the period. 
        
               randomBeacon.execRequestRandomSeedForNextPeriod(lastPeriod, newPeriod); 
        
               // Get all candidate ids 
        
               address[] memory allCids = _candidateIds; 
        
               _syncFastFinalityReward({ epoch: epoch, validatorIds: allCids }); 
        
               if (periodEnding) { 
        
                 ISlashIndicator slashIndicatorContract = ISlashIndicator(getContract(ContractType.SLASH_INDICATOR)); 
        
                 // Slash submit random beacon proof unavailability first, then update credit scores. 
        
                 randomBeacon.execRecordAndSlashUnavailability(lastPeriod, newPeriod, address(slashIndicatorContract), allCids); 
        
                 slashIndicatorContract.execUpdateCreditScores(allCids, lastPeriod); 
        
                 (uint256[] memory delegatorBlockMiningRewards, uint256[] memory delegatorFastFinalityRewards) = 
        
                   _distributeRewardToTreasuriesAndCalculateTotalDelegatorsReward(lastPeriod, allCids); 
        
                 _settleAndTransferDelegatingRewards( 
        
                   lastPeriod, allCids, delegatorBlockMiningRewards, delegatorFastFinalityRewards 
        
                 ); 
        
                 _tryRecycleLockedFundsFromEmergencyExits(); 
        
                 _recycleDeprecatedRewards(); 
        
                 address[] memory revokedCandidateIds = _syncCandidateSet(newPeriod); 
        
                 if (revokedCandidateIds.length > 0) { 
        
                   // Re-update `allCids` after unsatisfied candidates get removed. 
        
                   allCids = _candidateIds; 
        
                   slashIndicatorContract.execResetCreditScores(revokedCandidateIds); 
        
                 } 
        
                 // Wrap up the beacon period includes (1) finalizing the beacon proof, and (2) determining the validator list for the next period by new proof. 
        
                 // Should wrap up the beacon after unsatisfied candidates get removed. 
        
                 randomBeacon.execFinalizeBeaconAndPendingCids(lastPeriod, newPeriod, allCids); 
        
                 _periodEndBlock[lastPeriod] = block.number; 
        
                 _currentPeriodStartAtBlock = block.number + 1; 
        
               } 
        
               // Clear the previous validator set and block producer set before sync the new set from beacon. 
        
               _clearPreviousValidatorSetAndBlockProducerSet(); 
        
               // Query the new validator set for upcoming epoch from the random beacon contract. 
        
               // Save new set into the contract storage. 
        
               address[] memory newValidatorIds = _syncValidatorSet(randomBeacon, newPeriod, nextEpoch); 
        
               // Activate applicable validators into the block producer set. 
        
               _updateApplicableValidatorToBlockProducerSet(newPeriod, nextEpoch, newValidatorIds); 
        
               emit WrappedUpEpoch(lastPeriod, epoch, periodEnding); 
        
               _periodOf[nextEpoch] = newPeriod; 
        
               _lastUpdatedPeriod = newPeriod; 
        
             } 
        
           }

ID-5
Reentrancy in CandidateStaking.execDeprecatePools(address[],uint256):
External calls:
- ! _unsafeSendRONLimitGas(address(_pool.__shadowedPoolAdmin),deductingAmount,DEFAULT_ADDITION_GAS)
  - (success,None) = recipient.call{gas: gas,value: amount}()
- _unsafeSendRONLimitGas(address(_pool.__shadowedPoolAdmin),lastRewardAmount,DEFAULT_ADDITION_GAS)
  - (success,None) = recipient.call{gas: gas,value: amount}()
    State variables written after the call(s):
- delete _adminOfActivePoolMapping[_pool.__shadowedPoolAdmin]
  BaseStaking._adminOfActivePoolMapping can be used in cross function reentrancies:
- CandidateStaking.execDeprecatePools(address[],uint256)
- BaseStaking.getPoolAddressOf(address)
- BaseStaking.isAdminOfActivePool(address)
- lastRewardAmount = _claimReward(poolId,_pool.__shadowedPoolAdmin,newPeriod)
  - _reward.debited = 0
  - _reward.lastPeriod = lastPeriod
  - _reward.aRps = _stakingPool[poolId].aRps
    RewardCalculation._userReward can be used in cross function reentrancies:
- RewardCalculation._claimReward(address,address,uint256)
- RewardCalculation._getReward(address,address,uint256,uint256)
- RewardCalculation._syncUserReward(address,address,uint256)

dpos-contract/src/ronin/staking/CandidateStaking.sol

Lines 120 to 155 in 0b6d306

    
           function execDeprecatePools( 
        
             address[] calldata poolIds, 
        
             uint256 newPeriod 
        
           ) external override onlyContract(ContractType.VALIDATOR) { 
        
             if (poolIds.length == 0) { 
        
               return; 
        
             } 
        
             for (uint256 i = 0; i < poolIds.length;) { 
        
               address poolId = poolIds[i]; 
        
               PoolDetail storage _pool = _poolDetail[poolId]; 
        
               // Deactivate the pool admin in the active mapping. 
        
               delete _adminOfActivePoolMapping[_pool.__shadowedPoolAdmin]; 
        
               // Deduct and transfer the self staking amount to the pool admin. 
        
               uint256 deductingAmount = _pool.stakingAmount; 
        
               if (deductingAmount > 0) { 
        
                 _deductStakingAmount(_pool, deductingAmount); 
        
                 if (!_unsafeSendRONLimitGas(payable(_pool.__shadowedPoolAdmin), deductingAmount, DEFAULT_ADDITION_GAS)) { 
        
                   emit StakingAmountTransferFailed(_pool.pid, _pool.__shadowedPoolAdmin, deductingAmount, address(this).balance); 
        
                 } 
        
               } 
        
               // Settle the unclaimed reward and transfer to the pool admin. 
        
               uint256 lastRewardAmount = _claimReward(poolId, _pool.__shadowedPoolAdmin, newPeriod); 
        
               if (lastRewardAmount > 0) { 
        
                 _unsafeSendRONLimitGas(payable(_pool.__shadowedPoolAdmin), lastRewardAmount, DEFAULT_ADDITION_GAS); 
        
               } 
        
               unchecked { 
        
                 ++i; 
        
               } 
        
             } 
        
             emit PoolsDeprecated(poolIds); 
        
           }

shadowing-state

Impact: High
Confidence: High

ID-6
CandidateStaking.______gap shadows:
- BaseStaking.______gap
- RewardCalculation.______gap

dpos-contract/src/ronin/staking/CandidateStaking.sol

Line 27 in 0b6d306

uint256[48] ______gap;

ID-7
BaseStaking.______gap shadows:
- RewardCalculation.______gap

dpos-contract/src/ronin/staking/BaseStaking.sol

Line 40 in 0b6d306

uint256[49] private ______gap;

ID-8
CommonStorage.______gap shadows:

dpos-contract/src/ronin/validator/storage-fragments/CommonStorage.sol

Line 39 in 0b6d306

uint256[44] private ______gap;

ID-9
DelegatorStaking.______gap shadows:
- BaseStaking.______gap
- RewardCalculation.______gap

dpos-contract/src/ronin/staking/DelegatorStaking.sol

Line 13 in 0b6d306

uint256[50] private ______gap;

uninitialized-state

Impact: High
Confidence: High

ID-10
TimingStorage._firstTrackedPeriodEnd is never initialized. It is used in:
- TimingStorage.getPeriodEndBlock(uint256)

dpos-contract/src/ronin/validator/storage-fragments/TimingStorage.sol

Line 23 in 0b6d306

uint256 internal _firstTrackedPeriodEnd;

ID-11
RoninBaseFeeTreasury._info is never initialized. It is used in:
- RoninBaseFeeTreasury.cancel(bytes32)

dpos-contract/src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol

Line 46 in 0b6d306

mapping(bytes32 hash => ProposalInfo) internal _info;

ID-12
TimingStorage._numberOfBlocksInEpoch is never initialized. It is used in:

dpos-contract/src/ronin/validator/storage-fragments/TimingStorage.sol

Line 10 in 0b6d306

uint256 internal _numberOfBlocksInEpoch;

ID-13
TimingStorage._periodOf is never initialized. It is used in:
- TimingStorage.tryGetPeriodOfEpoch(uint256)

dpos-contract/src/ronin/validator/storage-fragments/TimingStorage.sol

Line 19 in 0b6d306

mapping(uint256 epoch => uint256 period) internal _periodOf;

ID-14
JailingStorage._miningRewardDeprecatedAtPeriod is never initialized. It is used in:
- JailingStorage._miningRewardDeprecatedById(address,uint256)

dpos-contract/src/ronin/validator/storage-fragments/JailingStorage.sol

Line 9 in 0b6d306

    
           mapping(address => mapping(uint256 => bool)) internal _miningRewardDeprecatedAtPeriod;

ID-15
TimingStorage._lastUpdatedBlock is never initialized. It is used in:
- TimingStorage.getLastUpdatedBlock()

dpos-contract/src/ronin/validator/storage-fragments/TimingStorage.sol

Line 12 in 0b6d306

uint256 internal _lastUpdatedBlock;

ID-16
ValidatorInfoStorageV2._validatorCount is never initialized. It is used in:

dpos-contract/src/ronin/validator/storage-fragments/ValidatorInfoStorageV2.sol

Line 18 in 0b6d306

uint256 internal _validatorCount;

ID-17
TimingStorage._lastUpdatedPeriod is never initialized. It is used in:

dpos-contract/src/ronin/validator/storage-fragments/TimingStorage.sol

Line 14 in 0b6d306

uint256 internal _lastUpdatedPeriod;

ID-18
JailingStorage._blockProducerJailedBlock is never initialized. It is used in:
- JailingStorage._getJailedTimeLeftAtBlockById(address,uint256)
- JailingStorage._isJailedAtBlockById(address,uint256)

dpos-contract/src/ronin/validator/storage-fragments/JailingStorage.sol

Line 16 in 0b6d306

mapping(address => uint256) internal _blockProducerJailedBlock;

ID-19
ValidatorInfoStorageV2._validatorMap is never initialized. It is used in:
- ValidatorInfoStorageV2._isBlockProducerById(address)

dpos-contract/src/ronin/validator/storage-fragments/ValidatorInfoStorageV2.sol

Line 22 in 0b6d306

mapping(address cid => bool isBlockProducer) internal _validatorMap;

ID-20
TimingStorage._periodEndBlock is never initialized. It is used in:
- TimingStorage.getPeriodEndBlock(uint256)

dpos-contract/src/ronin/validator/storage-fragments/TimingStorage.sol

Line 21 in 0b6d306

mapping(uint256 period => uint256 endedAtBlock) internal _periodEndBlock;

ID-21
ValidatorInfoStorageV2._validatorIds is never initialized. It is used in:

dpos-contract/src/ronin/validator/storage-fragments/ValidatorInfoStorageV2.sol

Line 20 in 0b6d306

mapping(uint256 idx => address cid) internal _validatorIds;

ID-22
TimingStorage._currentPeriodStartAtBlock is never initialized. It is used in:
- TimingStorage.currentPeriodStartAtBlock()

dpos-contract/src/ronin/validator/storage-fragments/TimingStorage.sol

Line 16 in 0b6d306

uint256 internal _currentPeriodStartAtBlock;

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol

huyhuynh3103 · 2024-11-29T07:37:41Z

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol

+  function _requireValidNonce(
+    uint256 nonce
+  ) internal view {
+    require(nonce == getGlobalNonce(), ErrInvalidNonce(getGlobalNonce(), nonce));


Should fetch global nonce once

since there will be no onchain failure, this could result in small bytecode increase but imo no gas will be saved even if we cache global nonce

huyhuynh3103 · 2024-11-29T07:50:25Z

src/interfaces/basefee-treasury/IBaseFeeTreasury.sol

+   * - `msg.sender` must be `proposal.executor`.
+   * - `hash` must be existed and passed (ProposalStatus must be Passed).
+   */
+  function execute(


Should we support bulk execution? Not sure if there are cases where multiple proposals need to be executed in single tx though

may depend on proposal.executor so no.

thaixuandang · 2024-11-29T08:01:55Z

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol

+  function _calcMinVoteAgainst(
+    uint256 totalVotePower
+  ) internal view returns (uint96 minVoteAgainst) {
+    uint256 diff = _threshold.denom - _threshold.num;
+    minVoteAgainst = ((totalVotePower * diff) / _threshold.denom).toUint96();
+    require(minVoteAgainst != 0, ErrZeroMinVotePower());


This function should be calculated by taking totalVotePower - _calcMinVoteFor. Because with your implementation, may _calcMinVoteFor + _calcMinVoteAgainst < totalVotePower, which leads to a situation where the proposal is canceled once the votes just meet _calcMinVoteAgainst, even though further voting could potentially exceed _calcMinVoteFor.

Agree. To be more specific, let’s say the total vote power is 31 and the threshold is 0.5 (num = 1, denom = 2). Both _calcMinVoteAgainst and _calcMinVoteFor would return 15. Even if the current power of voteFor is 16, the proposal would still be canceled

acked. However, if it is actually come in this case on production, I still think it is okay if it is cancelled.

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol

huyhuynh3103 · 2024-11-29T08:13:40Z

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol

+    require(
+      _computePeriod(profile.getId2RegisteredAt(cid) + COOLDOWN_PERIOD) <= _computePeriod($._createdAt),
+      ErrNewlyRegisteredCannotVote(cid)
+    );


This condition prevents new candidates from voting on past proposals. Is it as expected?
Even if someone registers as a candidate and waits a day, he still can’t vote on past proposals.

As expected.

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol

huyhuynh3103

Need to verify with protocol team whether adding balance to a contract without receive() function is possible at node layer

thaixuandang · 2024-11-29T11:20:54Z

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol

+      return false;
+    }
+
+    if ($._votePow.vFor < _calcMinVoteFor($._totalVotePow)) return false;


Note that in the case where totalVotePow = 101 and the threshold is 70%, if $._votePow.For = 70, it would not satisfy >= 101 * 70% = 70.7. To be precise, I believe the comparison should be whether $._votePow.vFor * denom >= totalVotePow * num. Similarly, for early proposal cancellation, we need to compare $._votePow.Against * denom > totalPow * (denom - num).

Co-authored-by: TuDo1403 <[email protected]>

github-actions bot added the automated PR label Nov 13, 2024

TuDo1403 force-pushed the implement-feature/basefee-treasury/impl branch from b381ba4 to 1c1ef24 Compare November 28, 2024 06:00

thaixuandang reviewed Nov 28, 2024

View reviewed changes

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol Outdated Show resolved Hide resolved

huyhuynh3103 reviewed Nov 28, 2024

View reviewed changes

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol Outdated Show resolved Hide resolved

huyhuynh3103 reviewed Nov 28, 2024

View reviewed changes

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol Outdated Show resolved Hide resolved

TuDo1403 reviewed Nov 28, 2024

View reviewed changes

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol Outdated Show resolved Hide resolved

huyhuynh3103 reviewed Nov 29, 2024

View reviewed changes

thaixuandang reviewed Nov 29, 2024

View reviewed changes

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol Outdated Show resolved Hide resolved

huyhuynh3103 reviewed Nov 29, 2024

View reviewed changes

src/ronin/basefee-treasury/RoninBaseFeeTreasury.sol Show resolved Hide resolved