hats-finance / Metrom-0xfdfc6d4ac5807d7460da20a3a1c0c84ef2b9c5a2

Smart contracts for the Metrom project.
GNU General Public License v3.0
0 stars 0 forks source link

Fee-on-Transfer Tokens in `createCampaigns` Function cause revert in `_processRewardClaim`. #1

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xd972c2f66bb2ea8981fdfbe887602d67b1b14a0e0f5c7d7aa917c3298cb29bb1 Severity: medium

Description: Description As mentioend by sponsers any ERC 20 is allowed, there are several ERC20 tokens that take a small fee on transfers/transferFroms (known as "fee-on-transfer" tokens). For these tokens, it should not be assumed that if you transfer x tokens to an address, the address actually receives x tokens. In the createCampaigns function, the amount used for calculations is assumed to be the amount transferred. However, the contract will have amount - fee in its balance, causing the _processRewardClaim function to revert and resulting in funds being stuck in the contract.

                uint256 _feeAmount = _amount * _fee / UNIT;
                uint256 _rewardAmountMinusFees = _amount - _feeAmount;
                claimableFees[_token] += _feeAmount;

                _feeAmounts[_j] = _feeAmount;
                _rewardAmountsMinusFees[_j] = _rewardAmountMinusFees;

                Reward storage reward = campaign.reward[_token];
                reward.amount = _rewardAmountMinusFees;
                reward.unclaimed = _rewardAmountMinusFees;

                IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);

Attack Scenario A user wants to create a campaign and uses fee-on-transfer tokens, setting the amount as 1000. The contract assumes 1000 tokens are transferred, but only 1000 - fee tokens are actually transferred to the contract, leading to potential reverts when _processRewardClaim is called.

Revised Code File (Optional)

Calculate the balance before and after the transfer and use the difference for the amount variable.

+                uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
+                IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
+                uint256 balanceAfter = IERC20(_token).balanceOf(address(this));
+                _amount = balanceAfter - balanceBefore;
                 uint256 _feeAmount = _amount * _fee / UNIT;
                 uint256 _rewardAmountMinusFees = _amount - _feeAmount;
                 claimableFees[_token] += _feeAmount;
@@ -172,8 +175,6 @@ contract Metrom is IMetrom {
                 Reward storage reward = campaign.reward[_token];
                 reward.amount = _rewardAmountMinusFees;
                 reward.unclaimed = _rewardAmountMinusFees;
-
-                IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
             }

By making these changes, the contract will correctly handle fee-on-transfer tokens, ensuring that the actual amount transferred is used for subsequent calculations, thus preventing potential reverts and ensuring funds are not stuck in the contract.

luzzif commented 4 months ago

This should have been addressed in https://github.com/metrom-xyz/contracts/commit/c883e364864961a17976c68f48b0779e23f4768b