`BribeUpgradeable` is not meant to handle fee-on-transfer tokens

[hats-bug-reporter[bot]]

Github username: @Rotcivegaf Twitter username: rotcivegaf Submission hash (on-chain): 0x2b5cf9568de5beb49effc2864f66cc6063bdbd8ce902c68a5a04c7ad4514e19e Severity: medium

Description: Lines:

• https://github.com/Satsyxbt/Fenix/blob/bfcee140a38b39f0eb34dea6921b3b7d5248df82/contracts/bribes/BribeUpgradeable.sol#L333
• https://github.com/Satsyxbt/Fenix/blob/bfcee140a38b39f0eb34dea6921b3b7d5248df82/contracts/bribes/BribeUpgradeable.sol#L342
• https://github.com/Satsyxbt/Fenix/blob/bfcee140a38b39f0eb34dea6921b3b7d5248df82/contracts/bribes/BribeUpgradeable.sol#L259-L327

Description:

Should a fee-on-transfer token be added as a reward token and deposited, the tokens will be locked in the BribeUpgradeable contract. Voters will be unable to withdraw their rewards.

Attack Scenario:

Tokens are deposited into the BribeUpgradeable contract using notifyRewardAmount, where amount of tokens are transferred, then added directly to rewardData[_rewardsToken][_startTimestamp].rewardsPerEpoch, but the amount received is less than the amount requested leaving this variable broken

For example:

• User calls notifyRewardAmount, with token as token that charges a 10% fee upon any transfer, and amount = 100
• _safeTransferFrom() only transfers 90 tokens to the contract due to the 10% fee
• Assuming epochRewards = 0, tokenRewardsPerEpoch[token][adjustedTstamp] becomes 100
• Later on, when getReward is called with the same token and epochStart rewardPerEpoch = tokenRewardsPerEpoch[token][epochStart] = 100 and safeTransfer attempts to transfer 100 tokens out of the contract
• However, the contract only contains 90 tokens
• getReward reverts

Recommended Mitigation Steps:

@@ -330,7 +330,10 @@ contract BribeUpgradeable is IBribe, BlastGovernorSetup, ReentrancyGuardUpgradea
     /// @dev    Rewards are saved into NEXT EPOCH mapping.
     function notifyRewardAmount(address _rewardsToken, uint256 reward) external nonReentrant {
         require(isRewardToken[_rewardsToken], "reward token not verified");
+
+        uint256 _before = IERC20Upgradeable(_rewardsToken).balanceOf(address(this));
         IERC20Upgradeable(_rewardsToken).safeTransferFrom(msg.sender, address(this), reward);
+        uint256 _after = IERC20Upgradeable(_rewardsToken).balanceOf(address(this));        

         uint256 _startTimestamp = IMinter(minter).active_period(); //period points to the current thursday. Bribes are distributed from next epoch (thursday)
         if (firstBribeTimestamp == 0) {
@@ -339,11 +342,11 @@ contract BribeUpgradeable is IBribe, BlastGovernorSetup, ReentrancyGuardUpgradea

         uint256 _lastReward = rewardData[_rewardsToken][_startTimestamp].rewardsPerEpoch;

-        rewardData[_rewardsToken][_startTimestamp].rewardsPerEpoch = _lastReward + reward;
+        rewardData[_rewardsToken][_startTimestamp].rewardsPerEpoch = _lastReward + _after - _before;
         rewardData[_rewardsToken][_startTimestamp].lastUpdateTime = block.timestamp;
         rewardData[_rewardsToken][_startTimestamp].periodFinish = _startTimestamp + WEEK;

-        emit RewardAdded(_rewardsToken, reward, _startTimestamp);
+        emit RewardAdded(_rewardsToken, _after - _before, _startTimestamp);
     }

[BohdanHrytsak]

Thank you for the submission.

If the support for such tokens was claimed, then this thing would indeed be valid. But I also want to note:

fee-on-transfer tokens are custom implementations The code where the problem exists is part of OOS Tokens must be approved by authorized users, which also mitigates this problem and reduces it to inattention in adding support for such tokens by authorized users

[rotcivegaf]

Could you tell where it is made clear that this type of tokens will not be used?

[BohdanHrytsak]

If I'm not mistaken, nowhere does it say that they also need to be maintained. In most cases, the severity level for this type of issue is recognized as low.

In addition, support for this type of token is not mandated by the Thena/Chronos code we follow and due to the lack of impact remains OOS