The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Tokens with a fee-on-transfer mechanism like PAXG, will break the protocol

t0x1c

Summary & Vulnerability Details

The Protocol is incompatible with tokens that have a fee-on-transfer mechanism. Such tokens for example are PAXG (officially supported as per documentation), while USDT has a built-in fee-on-transfer mechanism that is currently switched off.

One example of such code is in distributeAssets() while using safeTransferFrom() (L232). This will work incorrectly if the token has a fee-on-transfer mechanism due to following sequence of events -

    1. The rewards will be calculated based on the variable _portion on L227.

    1. However, the amount which is added to the contract via safeTransferFrom will only be _portion - fee on L232.

    1. Since now the funds in the contract are less than the sum of all rewards (less than the sum of all _portions), the first few users calling claimRewards may receive their full reward, but eventually subsequent users' call will start reverting due to insufficient funds.

    1. Also an additional note: When L175 makes the transfer of reward to the user, the user will actually only receive _rewardAmount - fee, but I think that can be signed off to as an acceptable effect/risk of using fee-on-transfer tokens.

LiquidationPool.sol#L232

function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
consolidatePendingStakes();
(,int256 priceEurUsd,,,) = Chainlink.AggregatorV3Interface(eurUsd).latestRoundData();
uint256 stakeTotal = getStakeTotal();
uint256 burnEuros;
uint256 nativePurchased;
for (uint256 j = 0; j < holders.length; j++) {
Position memory _position = positions[holders[j]];
uint256 _positionStake = stake(_position);
if (_positionStake > 0) {
for (uint256 i = 0; i < _assets.length; i++) {
ILiquidationPoolManager.Asset memory asset = _assets[i];
if (asset.amount > 0) {
(,int256 assetPriceUsd,,,) = Chainlink.AggregatorV3Interface(asset.token.clAddr).latestRoundData();
uint256 _portion = asset.amount * _positionStake / stakeTotal;
uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
* _hundredPC / _collateralRate;
if (costInEuros > _position.EUROs) {
_portion = _portion * _position.EUROs / costInEuros;
costInEuros = _position.EUROs;
}
_position.EUROs -= costInEuros;
@-------------------> rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += _portion;
burnEuros += costInEuros;
if (asset.token.addr == address(0)) {
nativePurchased += _portion;
} else {
@-------------------> IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
}
}
}
}
positions[holders[j]] = _position;
}
if (burnEuros > 0) IEUROs(EUROs).burn(address(this), burnEuros);
returnUnpurchasedNative(_assets, nativePurchased);
}

LiquidationPool.sol#L175)

function claimRewards() external {
ITokenManager.Token[] memory _tokens = ITokenManager(tokenManager).getAcceptedTokens();
for (uint256 i = 0; i < _tokens.length; i++) {
ITokenManager.Token memory _token = _tokens[i];
uint256 _rewardAmount = rewards[abi.encodePacked(msg.sender, _token.symbol)];
if (_rewardAmount > 0) {
delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
if (_token.addr == address(0)) {
(bool _sent,) = payable(msg.sender).call{value: _rewardAmount}("");
require(_sent);
} else {
@-------------> IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
}
}
​
}
}

Impact

  • Users unable to claim their reward

    • Impact: High, as some users will lose value

    • Likelihood: High, as such tokens (example: PAXG) are officially supported currently as per documentation

Tools Used

Manual inspection

Recommendations

  • Cache the balance before a transferFrom / transfer to the contract and then check it after the transfer and use the difference between them as the newly added balance.

    • In our context, it basically means that the _portion variable needs to be decreased by the token-transfer-fee before adding it to the rewards[] variable.

  • Also best to implement a nonReentrant modifier, as otherwise ERC777 tokens may manipulate this.

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

fee-on-transfer

00xSEV Auditor
over 1 year ago
cosine Auditor
over 1 year ago
0xasen Auditor
over 1 year ago
00xSEV Auditor
over 1 year ago
hrishibhat Lead Judge
over 1 year ago
hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Out of scope
Assigned finding tags:

fee-on-transfer

Support

FAQs

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