From 61ee665bd33accfbcaf93f29176aec2a587752e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Volpe?= Date: Fri, 25 Oct 2024 19:33:34 +0200 Subject: [PATCH] Fixed potential accounting error, removed duplicated code, made design more flexible --- .../protocol/contracts/common/FeeHandler.sol | 68 +++++++++++-------- .../test-sol/unit/common/FeeHandler.t.sol | 42 +++++++++++- 2 files changed, 80 insertions(+), 30 deletions(-) diff --git a/packages/protocol/contracts/common/FeeHandler.sol b/packages/protocol/contracts/common/FeeHandler.sol index 963c3810ef5..a9b7f033454 100644 --- a/packages/protocol/contracts/common/FeeHandler.sol +++ b/packages/protocol/contracts/common/FeeHandler.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: LGPL-3.0-only pragma solidity ^0.5.13; +import { console } from "forge-std/console.sol"; + import "openzeppelin-solidity/contracts/math/SafeMath.sol"; import "openzeppelin-solidity/contracts/math/Math.sol"; import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; @@ -72,7 +74,7 @@ contract FeeHandler is address public ignoreRenaming_carbonFeeBeneficiary; - uint256 public celoToBeBurned; // TODO deprecate + uint256 private celoToBeBurned; // TODO deprecate // This mapping can not be public because it contains a FixidityLib.Fraction // and that'd be only supported with experimental features in this @@ -147,6 +149,10 @@ contract FeeHandler is _setCarbonFraction(newFraction); } + function setDistributionAndBurnAmounts(address tokenAddress) external { + return _setDistributionAndBurnAmounts(tokenStates[tokenAddress], IERC20(tokenAddress)); + } + function changeOtherBeneficiaryAllocation( address beneficiary, uint256 _newFraction @@ -316,6 +322,10 @@ contract FeeHandler is return IERC20(token).transfer(recipient, value); } + function getCeloToBeBurned() external view returns (uint256) { + return getCeloTokenState().toBurn; + } + /** * @param token The address of the token to query. * @return The amount burned for a token. @@ -385,6 +395,10 @@ contract FeeHandler is return tokenStates[tokenAddress].toDistribute; } + function getTokenToBurn(address tokenAddress) external view returns (uint256) { + return tokenStates[tokenAddress].toDistribute; + } + function getCarbonFraction() external view returns (uint256) { return getCarbonFractionFixidity().unwrap(); } @@ -446,19 +460,15 @@ contract FeeHandler is function getActiveTokens() public view returns (address[] memory) { return activeTokens.values; } - // TODO to allow this function to be called publicly, it should - // keep track of amounts to be burned, like the Celo token does - function _setDistributionAmounts( - TokenState storage tokenState, - IERC20 token - ) internal returns (uint256) { + + function _setDistributionAndBurnAmounts(TokenState storage tokenState, IERC20 token) internal { uint256 balanceOfToken = token.balanceOf(address(this)); + console.log("balanceOfToken", balanceOfToken); uint256 balanceToProcess = balanceOfToken.sub(tokenState.toDistribute).sub(tokenState.toBurn); - - uint256 balanceToBurn = _setDistributeAfterBurn(tokenState, balanceToProcess); + console.log("balanceToProcess", balanceToProcess); + _setDistributeAfterBurn(tokenState, balanceToProcess); emit DistributionAmountSet(address(token), tokenState.toDistribute); - return balanceToBurn; } function _executePayment(address tokenAddress, address beneficiary, uint256 amount) internal { @@ -487,9 +497,9 @@ contract FeeHandler is .newFixed(balanceToProcess) .multiply(getBurnFractionFixidity()) .fromFixed(); - tokenState.toBurn.add(balanceToBurn); + tokenState.toBurn = tokenState.toBurn.add(balanceToBurn); tokenState.toDistribute = tokenState.toDistribute.add(balanceToProcess.sub(balanceToBurn)); - return balanceToBurn; + return tokenState.toBurn; } function checkTotalBeneficiary() internal view { @@ -554,17 +564,12 @@ contract FeeHandler is * @notice Burns all the Celo balance of this contract. */ function _burnCelo() private { - TokenState storage tokenState = tokenStates[getCeloTokenAddress()]; - ICeloToken celo = ICeloToken(getCeloTokenAddress()); + address celoTokenAddress = getCeloTokenAddress(); + TokenState storage tokenState = tokenStates[celoTokenAddress]; + _setDistributionAndBurnAmounts(tokenState, IERC20(celoTokenAddress)); - uint256 balanceOfCelo = address(this).balance; - - uint256 balanceToProcess = balanceOfCelo.sub(tokenState.toDistribute).sub(tokenState.toBurn); - uint256 balanceToBurn = _setDistributeAfterBurn(tokenState, balanceToProcess); - uint256 totalBalanceToBurn = balanceToBurn.add(tokenState.toBurn); - getCeloTokenState().toBurn = 0; - - celo.burn(totalBalanceToBurn); + ICeloToken(celoTokenAddress).burn(tokenState.toBurn); + tokenState.toBurn = 0; } /** @@ -633,12 +638,10 @@ contract FeeHandler is "Max slippage has to be set to sell token" ); - uint256 balanceToBurn = _setDistributionAmounts(tokenState, token); + _setDistributionAndBurnAmounts(tokenState, token); - // small numbers cause rounding errors and zero case should be skipped - if (balanceToBurn < MIN_BURN) { - return; - } + uint256 balanceToBurn = tokenState.toBurn; + console.log("balanceToBurn", balanceToBurn); if (dailySellLimitHit(tokenAddress, balanceToBurn)) { // in case the limit is hit, burn the max possible @@ -646,6 +649,11 @@ contract FeeHandler is emit DailyLimitHit(tokenAddress, balanceToBurn); } + // small numbers cause rounding errors and zero case should be skipped + if (balanceToBurn < MIN_BURN) { + return; + } + token.transfer(tokenState.handler, balanceToBurn); IFeeHandlerSeller handler = IFeeHandlerSeller(tokenState.handler); @@ -656,7 +664,9 @@ contract FeeHandler is FixidityLib.unwrap(tokenState.maxSlippage) ); - celoToBeBurned = celoToBeBurned.add(celoReceived); + // substract from toBurn only the amount that was burned + tokenState.toBurn = tokenState.toBurn.sub(balanceToBurn); + getCeloTokenState().toBurn = getCeloTokenState().toBurn.add(celoReceived); tokenState.pastBurn = tokenState.pastBurn.add(balanceToBurn); updateLimits(tokenAddress, balanceToBurn); @@ -733,9 +743,11 @@ contract FeeHandler is function _distributeAll() private { for (uint256 i = 0; i < EnumerableSet.length(activeTokens); i++) { address token = activeTokens.get(i); + _setDistributionAndBurnAmounts(tokenStates[token], IERC20(token)); _distribute(token); } // distribute Celo + _setDistributionAndBurnAmounts(getCeloTokenState(), IERC20(getCeloTokenAddress())); _distribute(getCeloTokenAddress()); } diff --git a/packages/protocol/test-sol/unit/common/FeeHandler.t.sol b/packages/protocol/test-sol/unit/common/FeeHandler.t.sol index 582e9cd15a9..3ef333e43af 100644 --- a/packages/protocol/test-sol/unit/common/FeeHandler.t.sol +++ b/packages/protocol/test-sol/unit/common/FeeHandler.t.sol @@ -691,13 +691,17 @@ contract FeeHandlerTest_SellMentoTokens_WhenTokenEnabled is FeeHandlerTest_SellM assertEq(stableToken.balanceOf(address(feeHandler)), balanceBefore); } + function resetLimit() internal { + skip(DAY); + } + function test_ResetSellLimitDaily() public { fundFeeHandlerStable(3000, address(stableToken), address(exchangeUSD)); feeHandler.setDailySellLimit(address(stableToken), 1000); feeHandler.sell(address(stableToken)); assertEq(stableToken.balanceOf(address(feeHandler)), 2000); - skip(DAY); + resetLimit(); feeHandler.sell(address(stableToken)); assertEq(stableToken.balanceOf(address(feeHandler)), 1000); } @@ -712,6 +716,39 @@ contract FeeHandlerTest_SellMentoTokens_WhenTokenEnabled is FeeHandlerTest_SellM assertEq(stableToken.balanceOf(address(feeHandler)), 2000); } + function test_HitLimitDoesntAffectAccounting() public { + fundFeeHandlerStable(3000, address(stableToken), address(exchangeUSD)); + feeHandler.setDailySellLimit(address(stableToken), 1000); + + feeHandler.sell(address(stableToken)); + assertEq(stableToken.balanceOf(address(feeHandler)), 2000); + + assertEq(feeHandler.getTokenToDistribute(address(stableToken)), 600); + + resetLimit(); + + feeHandler.sell(address(stableToken)); + assertEq(feeHandler.getTokenToDistribute(address(stableToken)), 600); + assertEq(stableToken.balanceOf(address(feeHandler)), 1000); + + resetLimit(); + + feeHandler.sell(address(stableToken)); + assertEq(feeHandler.getTokenToDistribute(address(stableToken)), 600); + assertEq(stableToken.balanceOf(address(feeHandler)), 600); + } + + function test_setDistributionAndBurnAmountsDoesntAffectBurn() public { + fundFeeHandlerStable(3000, address(stableToken), address(exchangeUSD)); + feeHandler.setDailySellLimit(address(stableToken), 1000); + + feeHandler.setDistributionAndBurnAmounts(address(stableToken)); + feeHandler.sell(address(stableToken)); + + assertEq(stableToken.balanceOf(address(feeHandler)), 2000); + assertEq(feeHandler.getTokenToDistribute(address(stableToken)), 600); + } + function test_Sell_WhenOtherTokenHitLimit() public { fundFeeHandlerStable(3000, address(stableToken), address(exchangeUSD)); feeHandler.setDailySellLimit(address(stableToken), 1000); @@ -746,7 +783,7 @@ contract FeeHandlerTest_SellMentoTokens_WhenTokenEnabled is FeeHandlerTest_SellM assertEq(feeHandler.getPastBurnForToken(address(stableToken)), 8e17); assertEq(stableToken.balanceOf(address(feeHandler)), 2e17); assertEq(feeHandler.getTokenToDistribute(address(stableToken)), 2e17); - assertEq(feeHandler.celoToBeBurned(), expectedCeloAmount); + assertEq(feeHandler.getCeloToBeBurned(), expectedCeloAmount); } function test_Reverts_WhenNotEnoughReports() public { @@ -1001,6 +1038,7 @@ contract FeeHandlerTest_HandleMentoTokens is FeeHandlerTestAbstract { celoToken.balanceOf(address(0x000000000000000000000000000000000000dEaD)), 398482170620712919 ); + assertEq(stableToken.balanceOf(address(feeHandler)), 0); } }