From 8906a6ef82759336070666fc7c29a6efd6713024 Mon Sep 17 00:00:00 2001 From: arcantheon <140178288+arcantheon@users.noreply.github.com> Date: Mon, 24 Jun 2024 13:12:34 +0530 Subject: [PATCH] Audit Fixes QS-17 and rewardRate precision bug (#70) * fix(Rewarder): added a function to call farm.recoverRewardFunds * Added test cases for recoverRewardFundsOfFarm of Rewarder contract * fix(NormalizeAmountRewarder): handle >18 precision and adjusts total value * Added and updated test cases * refactor(formatting): Rewarder * refactor(Rewarder): _normalizeAmount does not need to check decimals as it is added while configuration * perf(Rewarder): _normalizeAmount uses REWARD_TOKEN_DECIMALS directly from storage * Revert "perf(Rewarder): _normalizeAmount uses REWARD_TOKEN_DECIMALS directly from storage" This reverts commit c86cf2c401d0d7cfa1f5e35c65c6a41fbd17db24. * Updated test cases * perf(Rewarder): _normalizeAmount uses REWARD_TOKEN_DECIMALS directly from storage --------- Co-authored-by: Yash Pitroda --- contracts/rewarder/Rewarder.sol | 34 +++++++++++++++--------- test/rewarder/Rewarder.t.sol | 47 +++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/contracts/rewarder/Rewarder.sol b/contracts/rewarder/Rewarder.sol index 539aee2..873cfd2 100644 --- a/contracts/rewarder/Rewarder.sol +++ b/contracts/rewarder/Rewarder.sol @@ -73,6 +73,7 @@ contract Rewarder is Ownable, Initializable, ReentrancyGuard { uint256 public constant DENOMINATOR = 100; uint256 public constant ONE_YEAR = 365 days; address public REWARD_TOKEN; // solhint-disable-line var-name-mixedcase + uint8 public REWARD_TOKEN_DECIMALS; // solhint-disable-line var-name-mixedcase uint256 public totalRewardRate; // Rewards emitted per second for all the farms from this rewarder. address public rewarderFactory; // farm -> FarmRewardConfig. @@ -223,6 +224,7 @@ contract Rewarder is Ownable, Initializable, ReentrancyGuard { _validatePriceFeed(_rwdToken, _oracle); rewarderFactory = _rewarderFactory; REWARD_TOKEN = _rwdToken; + REWARD_TOKEN_DECIMALS = ERC20(_rwdToken).decimals(); _validateNonZeroAddr(_admin); _transferOwnership(_admin); } @@ -292,10 +294,13 @@ contract Rewarder is Ownable, Initializable, ReentrancyGuard { } // Getting reward token price to calculate rewards emission. priceData = _getPrice(REWARD_TOKEN, oracle); + + // For token with lower decimals the calculation of rewardRate might not be accurate because of precision loss in truncation. // rewardValuePerSecond = (APR * totalValue / 100) / 365 days. // rewardRate = rewardValuePerSecond * pricePrecision / price. rewardRate = (farmRewardConfig.apr * totalValue * priceData.precision) / (APR_PRECISION * DENOMINATOR * ONE_YEAR * priceData.price); + if (rewardRate > farmRewardConfig.maxRewardRate) { rewardRate = farmRewardConfig.maxRewardRate; } @@ -350,18 +355,6 @@ contract Rewarder is Ownable, Initializable, ReentrancyGuard { totalRewardRate = totalRewardRate - _oldRewardRate + _newRewardRate; } - /// @notice Function to normalize asset amounts to be of precision 1e18. - /// @param _token Address of the asset token. - /// @param _amount Amount of the token. - /// @return Normalized amount of the token in 1e18. - function _normalizeAmount(address _token, uint256 _amount) private returns (uint256) { - if (_decimals[_token] == 0) { - _decimals[_token] = ERC20(_token).decimals(); - } - _amount *= 10 ** (18 - _decimals[_token]); - return _amount; - } - /// @notice Function to validate farm. /// @param _farm Address of the farm to be validated. /// @param _baseTokens Array of base tokens. @@ -386,6 +379,7 @@ contract Rewarder is Ownable, Initializable, ReentrancyGuard { hasBaseTokens = false; for (uint8 j; j < _assetsLen;) { if (_baseTokens[i] == _assets[j]) { + _decimals[_baseTokens[i]] = ERC20(_baseTokens[i]).decimals(); hasBaseTokens = true; farmRewardConfigs[_farm].baseAssetIndexes.push(j); // Deleting will make _assets[j] -> 0x0 so if _baseTokens have repeated address, this function will return false. @@ -406,6 +400,22 @@ contract Rewarder is Ownable, Initializable, ReentrancyGuard { return true; } + /// @notice Function to normalize asset amounts to be of precision REWARD_TOKEN_DECIMALS. + /// @param _token Address of the asset token. + /// @param _amount Amount of the token. + /// @return Normalized amount of the token in _desiredPrecision. + function _normalizeAmount(address _token, uint256 _amount) private view returns (uint256) { + uint8 decimals = _decimals[_token]; + uint8 rwdTokenDecimals = REWARD_TOKEN_DECIMALS; + if (decimals < rwdTokenDecimals) { + return _amount * 10 ** (rwdTokenDecimals - decimals); + } + if (decimals > rwdTokenDecimals) { + return _amount / 10 ** (decimals - rwdTokenDecimals); + } + return _amount; + } + /// @notice Function to fetch and get the price of a token. /// @param _token Token for which the the price is to be fetched. /// @param _oracle Address of the oracle contract. diff --git a/test/rewarder/Rewarder.t.sol b/test/rewarder/Rewarder.t.sol index 93b71e0..edae5bd 100644 --- a/test/rewarder/Rewarder.t.sol +++ b/test/rewarder/Rewarder.t.sol @@ -120,7 +120,7 @@ contract TestUpdateAPR is RewarderTest { } function test_UpdateAPR_CapRewardsWithMaxRwdRate() public useKnownActor(rewardManager) { - uint256 MAX_REWARD_RATE = 166665; + uint256 MAX_REWARD_RATE = 50; // 50 wei: Because rwdToken decimals are 6 Rewarder.FarmRewardConfigInput memory rewardConfig; address[] memory baseAssets = new address[](1); baseAssets[0] = USDCe; @@ -133,12 +133,55 @@ contract TestUpdateAPR is RewarderTest { rewarder.updateRewardConfig(lockupFarm, rewardConfig); changePrank(owner); CamelotV2Farm(lockupFarm).updateRewardData(USDCe, address(rewarder)); - deposit(lockupFarm, false, 1000); + deposit(lockupFarm, false, 100000); rewarder.calibrateReward(lockupFarm); (, uint256 rewardRate,,) = rewarder.farmRewardConfigs(lockupFarm); assertEq(rewardRate, MAX_REWARD_RATE); } + function test_UpdateAPR_ForBaseTokenDecimalsMoreThanRwdTokenDecimals() public useKnownActor(rewardManager) { + rewarder = Rewarder(rewarderFactory.deployRewarder(USDCe)); + Rewarder.FarmRewardConfigInput memory rewardConfig; + address[] memory baseAssets = new address[](1); + baseAssets[0] = DAI; + rewardConfig = Rewarder.FarmRewardConfigInput({ + apr: 12e8, + maxRewardRate: UINT256_MAX, + baseTokens: baseAssets, + nonLockupRewardPer: 5000 + }); + rewarder.updateRewardConfig(lockupFarm, rewardConfig); + changePrank(owner); + CamelotV2Farm(lockupFarm).updateRewardData(USDCe, address(rewarder)); + deposit(lockupFarm, false, 1000); + rewarder.calibrateReward(lockupFarm); + (, uint256 rewardRate,,) = rewarder.farmRewardConfigs(lockupFarm); + assertTrue((rewardRate * 30 days) / 1e6 > 0); + assertEq((rewardRate * 30 days) / 1e9, 0); + } + + function test_UpdateAPR_ForRwdTokenDecimalsMoreThanBaseTokenDecimals() public useKnownActor(rewardManager) { + vm.mockCall(USDCe, abi.encodeWithSelector(ERC20.decimals.selector), abi.encode(20)); + rewarder = Rewarder(rewarderFactory.deployRewarder(USDCe)); + vm.clearMockedCalls(); + Rewarder.FarmRewardConfigInput memory rewardConfig; + address[] memory baseAssets = new address[](1); + baseAssets[0] = DAI; + rewardConfig = Rewarder.FarmRewardConfigInput({ + apr: 12e8, + maxRewardRate: UINT256_MAX, + baseTokens: baseAssets, + nonLockupRewardPer: 5000 + }); + rewarder.updateRewardConfig(lockupFarm, rewardConfig); + changePrank(owner); + CamelotV2Farm(lockupFarm).updateRewardData(USDCe, address(rewarder)); + deposit(lockupFarm, false, 1000); + rewarder.calibrateReward(lockupFarm); + (, uint256 rewardRate,,) = rewarder.farmRewardConfigs(lockupFarm); + assertTrue((rewardRate * 30 days) / 1e20 > 0); + } + function _setupFarmRewards() private { Rewarder.FarmRewardConfigInput memory rewardConfig; address[] memory baseAssets = new address[](1);