Summary
The protocol does not correctly account for fee-on-transfer tokens even though PAXG is in scope and has a fee-on-transfer. You can see its implementation contract here and check the code on line 370-380.
Vulnerability Details
This can cause many problems, if not accounted for properly. Let's take a look at the runLiquidation function in LiquidationPoolManager:
function runLiquidation(uint256 _tokenId) external {
ISmartVaultManager manager = ISmartVaultManager(smartVaultManager);
manager.liquidateVault(_tokenId);
distributeFees();
ITokenManager.Token[] memory tokens = ITokenManager(manager.tokenManager()).getAcceptedTokens();
ILiquidationPoolManager.Asset[] memory assets = new ILiquidationPoolManager.Asset[](tokens.length);
uint256 ethBalance;
for (uint256 i = 0; i < tokens.length; i++) {
ITokenManager.Token memory token = tokens[i];
if (token.addr == address(0)) {
ethBalance = address(this).balance;
if (ethBalance > 0)
assets[i] = ILiquidationPoolManager.Asset(token, ethBalance);
} else {
IERC20 ierc20 = IERC20(token.addr);
uint256 erc20balance = ierc20.balanceOf(address(this));
if (erc20balance > 0) {
assets[i] = ILiquidationPoolManager.Asset(token, erc20balance);
ierc20.approve(pool, erc20balance);
}
}
}
LiquidationPool(pool).distributeAssets{value: ethBalance}(assets, manager.collateralRate(), manager.HUNDRED_PC()); <--
forwardRemainingRewards(tokens);
}
The contract is giving approvals to the LiquidationPool and then calling its distributeAssets function:
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);
}
As you can see, on this line:
rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += _portion;
we're accounting for the rewards of the holder, by incrementing it with _portion and then we're transferring the required rewards from the manager to address(this):
IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
The problem is that we accounted for the rewards by incrementing with _portion but the contract will actually receive _portion - fee because of the fee that is taken on the transfer.
Which in turn will cause problems in the claimRewards function:
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); <--
}
}
}
}
This function will try to transfer _rewardAmount:
IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
However, the contract received less than _rewardAmount in the previous function. Which will lead to:
the function reverts and the user can't claim his rewards. This is the case if there are no other users with rewards from the same token
or
he'll be able to claim but he'll claim more than he was supposed to, so at some point, a situation will be reached where someone else can't claim his rewards as there is not enough balance in the contract.
Impact
Stuck user funds.
Tools Used
Manual review
Recommendations
Consider comparing the balance of the contract before and after the transfer to get the actual transferred amount. Like this:
balanceBefore = IERC20(asset.token.addr).balanceOf(address(this));
IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
balanceAfter = IERC20(asset.token.addr).balanceOf(address(this));
transferredAmount = balanceAfter - balanceBefore;
rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += transferredAmount;
