Tadle

Tadle
DeFiFoundry
27,750 USDC
View results
Submission Details
Severity: medium
Valid

Unnecessary balance checks and precision issues in TokenManager::_transfer

Summary

The TokenManager::_transfer function performs unnecessary balance checks before and after the transfer. Additionally, it uses exact equality checks for balance differences, which can cause issues with tokens that have transfer fees or unusual rounding behaviors.

Vulnerability Details

The _transfer function performs balance checks that are typically handled by the ERC20 token itself:

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
// ...
@> if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
@> if (toBalanceAft != toBalanceBef + _amount) {
// ...

These checks can cause issues with tokens that have transfer fees or non-standard implementations. The test case demonstrates this issue: (to be inserted in PreMarkets.t.sol)

import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {Rescuable} from "../src/utils/Rescuable.sol";
// Mock token contract with a 1% transfer fee
contract MockTokenWithFee is ERC20 {
constructor() ERC20("MockFeeToken", "MFT") {}
function approve(
address spender,
uint256 amount
) public virtual override returns (bool) {
_approve(_msgSender(), spender, amount);
return true;
}
function transfer(
address recipient,
uint256 amount
) public virtual override returns (bool) {
uint256 fee = amount / 100; // 1% fee
uint256 amountAfterFee = amount - fee;
_transfer(_msgSender(), recipient, amountAfterFee);
_transfer(_msgSender(), address(this), fee); // Fee goes to the contract
return true;
}
function transferFrom(
address sender,
address recipient,
uint256 amount
) public virtual override returns (bool) {
uint256 fee = amount / 100; // 1% fee
uint256 amountAfterFee = amount - fee;
_transfer(sender, recipient, amountAfterFee);
_transfer(sender, address(this), fee); // Fee goes to the contract
uint256 currentAllowance = allowance(sender, _msgSender());
require(
currentAllowance >= amount,
"ERC20: transfer amount exceeds allowance"
);
unchecked {
_approve(sender, _msgSender(), currentAllowance - amount);
}
return true;
}
}
function testPrecisionIssue() public {
// Setup
uint256 initialBalance = 100 * 10 ** 18; // 100 tokens
// Deploy a mock token with a 1% transfer fee
MockTokenWithFee mockToken = new MockTokenWithFee();
vm.prank(user);
mockToken.approve(address(tokenManager), initialBalance);
deal(address(mockToken), address(user), initialBalance);
// Atempting to "till in" fails
vm.expectRevert(Rescuable.TransferFailed.selector);
vm.prank(address(preMarktes));
tokenManager.tillIn(user, address(mockToken), initialBalance, true);
// Check balances
uint256 userBalance = mockToken.balanceOf(user);
assertEq(
userBalance,
initialBalance,
"User's balance should be the same as the initial balance"
);
uint256 userTokenBalance = tokenManager.userTokenBalanceMap(
user,
address(mockToken),
TokenBalanceType.SalesRevenue
);
assertEq(
userTokenBalance,
0,
"User's balance in TokenManager should be 0"
);
assertEq(
mockToken.balanceOf(address(capitalPool)),
0,
"The balance of the token in the capital pool should be 0"
);
}

Impact

  • Potential for failed transfers when using tokens with transfer fees or non-standard implementations.

  • Users may be unable to withdraw their funds if using tokens with transfer fees.

  • Users may be unable to send their funds if using tokens with transfer fees.

Tools Used

Manual Review - Testing

Recommendations

  1. Remove the unnecessary balance checks in the _transfer function.

  2. Instead of using exact equality checks, implement a tolerance threshold for balance differences to account for potential transfer fees or rounding issues.

  3. Consider using OpenZeppelin's SafeERC20 library for safer token transfers.

Here's an example of how the _transfer function could be improved:

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
- uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
- uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
+ uint256 balanceBefore = IERC20(_token).balanceOf(_to);
_safe_transfer_from(_token, _from, _to, _amount);
+ uint256 balanceAfter = IERC20(_token).balanceOf(_to);
- uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);
- uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
+ uint256 receivedAmount = balanceAfter - balanceBefore;
+ require(receivedAmount > 0, "Transfer failed");
}
Updates

Lead Judging Commences

0xnevi Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-FOT-Rebasing

Valid medium, there are disruptions to the ability to take market actions. The following functions will be disrupted without the possibiliy of reaching settlement, since the respective offers cannot be created/listed regardless of mode when transferring collateral token required to the CapitalPool contract or when refunding token from user to capital pool during relisting. So withdrawal is not an issue - `createOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L96-L102) - `listOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L355-L362) - `relistOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L515-L521) - `createTaker()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L831-L836) I believe medium severity is appropriate although the likelihood is high and impact is medium (only some level of disruption i.e. FOT tokens not supported and no funds at risk)

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.