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

M-2 Tokens with a fee-on-transfer mechanism will cause claimRewards to revert

agadzhalov

Summary

Some ERC20 tokens(e.g. STA,PAXG) charge a fee any time transfer() or transferFrom() is called and there are some that might in the future (USDC,USDT).
This will cause a difference between the value stored in rewards[abi.encodePacked(_position.holder, asset.token.symbol)] and the actual amount of the token in the contract.

Vulnerability Details

Example:
portion is 100, but 0.2% are paid for fee and the actual amount sent to the LiquidationPool contract is 98 (https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L232) but portion is still cached as 100 in rewards. (https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L227)
When the user tries to claim his rewards by calling claimRewards() _rewardAmount is going to be 100 (https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L168), it will pass the if condition if is bigger than 0,
but it will revert in the transfer logic as the actual amount is 98 (https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L175)

PoC

The rewardAmount is set at 227L in LiqudationPool.distributeAssets(), then a portion of the token is sent at 232L but it's not taken in account the 0.2% transfer fee

.......
rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += _portion; // rewardAmount is set to be 100
burnEuros += costInEuros;
if (asset.token.addr == address(0)) {
nativePurchased += _portion;
} else {
// portion is 100, 0.2% are paid for fee and the actual amount sent to the LiquidationPool contract is 98
// but portions is still cached as 100 in rewards
IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
}
......
}

Then when a user tries to claim his rewards by calling LiqudationPool.claimRewards() there will be a difference between the value stored in rewards[abi.encodePacked(_position.holder, asset.token.symbol)] and the actual amount of the token in the contract

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)]; // rewardAmount is 100, but the actual amount is 98
if (_rewardAmount > 0) { // it will be bigger than 0 but won't be able to transfer and it will revert
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); // it will revert here
}
}
​
}
}

Impact

As it might happen only on weird tokens which charges a fee-on-transfer I consider it as a Medium.

Tools Used

Manual review

Recommendations

Pre-calculating the fee, so the _rewardAmount has the correct value

Updates

Lead Judging Commences

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

fee-on-transfer

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.