From 53e5bc02b1dd4735d4c2f6370fc48c3dc6ae6664 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 28 Nov 2023 17:41:00 -0500 Subject: [PATCH 1/6] add regression test, confirmed fails on previous code --- .../compoundv3/CusdcV3Wrapper.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/plugins/individual-collateral/compoundv3/CusdcV3Wrapper.test.ts b/test/plugins/individual-collateral/compoundv3/CusdcV3Wrapper.test.ts index b47b71bff9..cbbd48ed43 100644 --- a/test/plugins/individual-collateral/compoundv3/CusdcV3Wrapper.test.ts +++ b/test/plugins/individual-collateral/compoundv3/CusdcV3Wrapper.test.ts @@ -598,6 +598,18 @@ describeFork('Wrapped CUSDCv3', () => { ) }) + it('regression test: able to claim rewards even when they are big without overflow', async () => { + // Nov 28 2023: uint64 math in CusdcV3Wrapper contract results in overflow when COMP rewards are even moderately large + + const compToken = await ethers.getContractAt('ERC20Mock', COMP) + expect(await compToken.balanceOf(wcusdcV3.address)).to.equal(0) + await advanceTime(1000) + await enableRewardsAccrual(cusdcV3, bn('2e18')) // enough to revert on uint64 implementation + + await expect(wcusdcV3.connect(bob).claimRewards()).to.emit(wcusdcV3, 'RewardsClaimed') + expect(await compToken.balanceOf(bob.address)).to.be.greaterThan(0) + }) + it('claims rewards and sends to claimer (claimTo)', async () => { const compToken = await ethers.getContractAt('ERC20Mock', COMP) expect(await compToken.balanceOf(wcusdcV3.address)).to.equal(0) From 5513e654ca00855929409c6e391533dd05ad1eff Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 28 Nov 2023 17:48:27 -0500 Subject: [PATCH 2/6] implement fix --- contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol | 8 +++----- contracts/plugins/assets/compoundv3/ICusdcV3Wrapper.sol | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol index 9234322186..39be4511de 100644 --- a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol +++ b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol @@ -26,8 +26,8 @@ contract CusdcV3Wrapper is ICusdcV3Wrapper, WrappedERC20, CometHelpers { ICometRewards public immutable rewardsAddr; IERC20 public immutable rewardERC20; - mapping(address => uint64) public baseTrackingIndex; - mapping(address => uint64) public baseTrackingAccrued; + mapping(address => uint64) public baseTrackingIndex; // keep uint64 to stay consistent with comet + mapping(address => uint256) public baseTrackingAccrued; // uint256 to avoid overflow mapping(address => uint256) public rewardsClaimed; constructor( @@ -286,9 +286,7 @@ contract CusdcV3Wrapper is ICusdcV3Wrapper, WrappedERC20, CometHelpers { (, uint64 trackingSupplyIndex) = getSupplyIndices(); uint256 indexDelta = uint256(trackingSupplyIndex - baseTrackingIndex[account]); - baseTrackingAccrued[account] += safe64( - (safe104(accountBal) * indexDelta) / TRACKING_INDEX_SCALE - ); + baseTrackingAccrued[account] += (safe104(accountBal) * indexDelta) / TRACKING_INDEX_SCALE; baseTrackingIndex[account] = trackingSupplyIndex; } diff --git a/contracts/plugins/assets/compoundv3/ICusdcV3Wrapper.sol b/contracts/plugins/assets/compoundv3/ICusdcV3Wrapper.sol index f1514ec8e8..89a9dcfb35 100644 --- a/contracts/plugins/assets/compoundv3/ICusdcV3Wrapper.sol +++ b/contracts/plugins/assets/compoundv3/ICusdcV3Wrapper.sol @@ -51,7 +51,7 @@ interface ICusdcV3Wrapper is IWrappedERC20, IRewardable { function convertDynamicToStatic(uint256 amount) external view returns (uint104); - function baseTrackingAccrued(address account) external view returns (uint64); + function baseTrackingAccrued(address account) external view returns (uint256); function baseTrackingIndex(address account) external view returns (uint64); From 1d627c71d5dcefd154a580d5d72b1a6769606d13 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 28 Nov 2023 18:01:43 -0500 Subject: [PATCH 3/6] slightly improve comments --- contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol index 39be4511de..4ee11c4add 100644 --- a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol +++ b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol @@ -26,8 +26,8 @@ contract CusdcV3Wrapper is ICusdcV3Wrapper, WrappedERC20, CometHelpers { ICometRewards public immutable rewardsAddr; IERC20 public immutable rewardERC20; - mapping(address => uint64) public baseTrackingIndex; // keep uint64 to stay consistent with comet - mapping(address => uint256) public baseTrackingAccrued; // uint256 to avoid overflow + mapping(address => uint64) public baseTrackingIndex; // uint64 to stay consistent with CometHelpers + mapping(address => uint256) public baseTrackingAccrued; // uint256 to avoid overflow in L:199 mapping(address => uint256) public rewardsClaimed; constructor( From 5ddfd9e8dad0cc40e44dfac3da396ef4cf3aa00f Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Wed, 29 Nov 2023 18:11:34 -0500 Subject: [PATCH 4/6] integrate julian feedback --- contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol index 4ee11c4add..1b55f69a97 100644 --- a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol +++ b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol @@ -257,7 +257,8 @@ contract CusdcV3Wrapper is ICusdcV3Wrapper, WrappedERC20, CometHelpers { uint256 indexDelta = uint256(trackingSupplyIndex - baseTrackingIndex[account]); uint256 newBaseTrackingAccrued = baseTrackingAccrued[account] + - safe64((safe104(balanceOf(account)) * indexDelta) / TRACKING_INDEX_SCALE); + (safe104(balanceOf(account)) * indexDelta) / + TRACKING_INDEX_SCALE; uint256 claimed = rewardsClaimed[account]; uint256 accrued = newBaseTrackingAccrued * RESCALE_FACTOR; From 289768df9dbf428b15b96917225b24e55e740b01 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Wed, 29 Nov 2023 18:12:42 -0500 Subject: [PATCH 5/6] lint clean --- contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol | 2 +- .../plugins/assets/yearnv2/YearnV2CurveFiatCollateral.sol | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol index 1b55f69a97..4b98f2c89b 100644 --- a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol +++ b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol @@ -26,7 +26,7 @@ contract CusdcV3Wrapper is ICusdcV3Wrapper, WrappedERC20, CometHelpers { ICometRewards public immutable rewardsAddr; IERC20 public immutable rewardERC20; - mapping(address => uint64) public baseTrackingIndex; // uint64 to stay consistent with CometHelpers + mapping(address => uint64) public baseTrackingIndex; // uint64 for consistency with CometHelpers mapping(address => uint256) public baseTrackingAccrued; // uint256 to avoid overflow in L:199 mapping(address => uint256) public rewardsClaimed; diff --git a/contracts/plugins/assets/yearnv2/YearnV2CurveFiatCollateral.sol b/contracts/plugins/assets/yearnv2/YearnV2CurveFiatCollateral.sol index 89b11d861c..9121982562 100644 --- a/contracts/plugins/assets/yearnv2/YearnV2CurveFiatCollateral.sol +++ b/contracts/plugins/assets/yearnv2/YearnV2CurveFiatCollateral.sol @@ -77,11 +77,15 @@ contract YearnV2CurveFiatCollateral is CurveStableCollateral { return (low, high, 0); } + // solhint-disable no-empty-blocks + /// DEPRECATED: claimRewards() will be removed from all assets and collateral plugins function claimRewards() external virtual override { // No rewards to claim, everything is part of the pricePerShare } + // solhint-enable no-empty-blocks + // === Internal === /// @return {ref/tok} Actual quantity of whole reference units per whole collateral tokens From 7e21b3f387fe9d0707b649c38d75a7afce4363ae Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Wed, 29 Nov 2023 18:12:56 -0500 Subject: [PATCH 6/6] RESCALE_FACTOR uint256 --- contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol index 4b98f2c89b..5b7b176061 100644 --- a/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol +++ b/contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol @@ -20,7 +20,7 @@ contract CusdcV3Wrapper is ICusdcV3Wrapper, WrappedERC20, CometHelpers { /// From cUSDCv3, used in principal <> present calculations uint256 public constant TRACKING_INDEX_SCALE = 1e15; /// From cUSDCv3, scaling factor for USDC rewards - uint64 public constant RESCALE_FACTOR = 1e12; + uint256 public constant RESCALE_FACTOR = 1e12; CometInterface public immutable underlyingComet; ICometRewards public immutable rewardsAddr;