The Standard

Summary

Because fee-on-transfer tokens are not specifically handled, LiquidationPool::distributeAssets() incorrectly calculates rewards for holders.
This results in some holders receiving more rewards than they should while others will be unable to claim any rewards for any token if their rewards amount of the fee-on-transfer token is greater than the remaining balance in LiquidationPool.

Vulnerability Details

For fee-on-transfer tokens, when an amount X is sent from one account to another, the actual amount received is X - f, where f is a fee.
In distributeAssets() this is not taken into account, so each holder is being allocated X rewards in reality the contract only receives X-f.
This is particularly relevant as the protocol intends to allow users deposit the PAXG fee-on-transfer token as collateral, a token which charges 0.02% fee per on-chain transaction; see here

The relevant lines from distributeAssets() are indicated below. The function is looping through all holders and assigning portions of the assets being distributed from a liquidated position to each eligible holder.
First, a _portion amount of each token is calculated for an eligible holder, X in the previous explanation. Second, the rewards mapping storage variable is updated for that holder by _portion amount of the token. Finally, _portion amount of the token is sent from the LiquidationPoolManager contract to the LiquidationPool.

When holders subsequently call LiquidationPool::claimRewards() the function will revert if the amount of the fee-on-transfer token available in it's balances is less than the _rewardAmount due to the customer.

Impact

Some holders will receive more fee-on-transfer rewards than they are entitled to while others will be unable to claim rewards for any tokens; as fee-on-transfer rewards are claimed the amount of people unable to claim will increase as there are less and less funds to withdraw.
As the protocol plans on allowing PAXG which implements a 0.02% fee on transfers, this represents a serious risk to the functioning of the protocol.

POC

To test, we need to update the code in 4 places:

1. Create a contract representing a fee-on-transfer token in contract/utils called paxgMock.sol:

   paxgMock.sol

   // SPDX-License-Identifier: UNLICENSED

   pragma solidity 0.8.17;

   import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

   contract paxgMock is ERC20 {

   uint8 private dec;

   address public feeCollector = address(this); // Address to collect fees

   uint256 public constant FEE_RATE = 20; // Fee rate (0.02%)

   constructor(string memory _name, string memory _symbol, uint8 _decimals)ERC20 (_name, _symbol) {

   dec = _decimals;

   }

   function mint(address to, uint256 amount) public {

   _mint(to, amount);

   }

   function decimals() public view override returns (uint8) {

   return dec;

   }

   function _transfer(address sender, address recipient, uint256 amount)internal override {

   uint256 fee = (amount * FEE_RATE) / 100000; // Calculate fee (002% of the amount)

   uint256 amountAfterFee = amount - fee;

   super._transfer(sender, feeCollector, fee); // Transfer fee

   super._transfer(sender, recipient, amountAfterFee); // Transferremaining amount

   }

   }

2. Update the mockTokenManager const in common.js, where we mock the tokenManager contract, to configure the fee-on-transfer token:

   Update Common.js

   const mockTokenManager = async (_) => {

   const MockERC20Factory = await ethers.getContractFactory("ERC20Mock");

   WBTC = await MockERC20Factory.deploy("Wrapped Bitcoin", "WBTC", 8);

   USDC = await MockERC20Factory.deploy("USD Coin", "USDC", 6);

   + const paxgMockFactory = await ethers.getContractFactory("paxgMock");

   + PAXG = await paxgMockFactory.deploy("Paxos Gold", "PAXG", 18);

   const EthUsd = await (

   await ethers.getContractFactory("ChainlinkMock")

   ).deploy("ETH/USD"); // $1900

   await EthUsd.setPrice(DEFAULT_ETH_USD_PRICE);

   const WbtcUsd = await (

   await ethers.getContractFactory("ChainlinkMock")

   ).deploy("WBTC/USD"); // $35,000

   await WbtcUsd.setPrice(DEFAULT_WBTC_USD_PRICE);

   const UsdcUsd = await (

   await ethers.getContractFactory("ChainlinkMock")

   ).deploy("USDC/USD"); // 1$

   await UsdcUsd.setPrice(DEFAULT_USDC_USD_PRICE);

   const TokenManager = await (

   await ethers.getContractFactory("TokenManagerMock")

   ).deploy(ethers.utils.formatBytes32String("ETH"), EthUsd.address);

   await TokenManager.addAcceptedToken(WBTC.address, WbtcUsd.address);

   await TokenManager.addAcceptedToken(USDC.address, UsdcUsd.address);

   + await TokenManager.addAcceptedToken(PAXG.address, UsdcUsd.address);

   - return { TokenManager, WBTC, USDC };

   + return { TokenManager, WBTC, USDC, PAXG };

   };

3. In liquidationPoolManager.js create new variable PAXG and import PAXG from mockTokenManager():

   "runLiquidation() DOS"

   describe("LiquidationPoolManager", async () => {

   let LiquidationPoolManager,

   LiquidationPoolManagerContract,

   LiquidationPool,

   SmartVaultManager,

   TokenManager,

   TST,

   EUROs,

   WBTC,

   USDC,

   holder1,

   holder2,

   holder3,

   holder4,

   holder5,

   Protocol,

   ERC20MockFactory,

   + PAXG;

   ​

   beforeEach(async () => {

   [holder1, holder2, holder3, holder4, holder5, Protocol] =

   await ethers.getSigners();

   ERC20MockFactory = await ethers.getContractFactory("ERC20Mock");

   TST = await ERC20MockFactory.deploy("The Standard Token", "TST", 18);

   EUROs = await (await ethers.getContractFactory("EUROsMock")).deploy();

   - ({ TokenManager, WBTC, USDC } = await mockTokenManager());

   + ({ TokenManager, WBTC, USDC, PAXG } = await mockTokenManager());

4. Finally, add the following test to liquidationPoolManager.js and run:

   "runLiquidation() DOS"

   describe.only("runLiquidation", async () => {

   const discounted = (amount) => {

   return amount.mul(HUNDRED_PC).div(DEFAULT_COLLATERAL_RATE);

   };

   ​

   const reverseDiscounted = (amount) => {

   return amount.mul(DEFAULT_COLLATERAL_RATE).div(HUNDRED_PC);

   };

   ​

   const scaleUpFrom = (amount, dec) => {

   return amount.mul(BigNumber.from(10).pow(18 - dec));

   };

   ​

   const scaleDownTo = (amount, dec) => {

   return amount.div(BigNumber.from(10).pow(18 - dec));

   };

   ​

   it("runs liquidations, and reverts if nothing to liquidate", async () => {

   await expect(

   LiquidationPoolManager.runLiquidation(TOKEN_ID)

   ).to.be.revertedWith("vault-not-undercollateralised");

   });

   ​

   + it.only("runLiquidation() DOS", async () => {

   + // the SmartVaultManager used here is a mock of the actual one; mocking

   + // the state changes which happen when runLiquidation() calls liquidateVault()

   ​

   + // add assets to SmartVaultManager which will be sent LiquidationPoolManager f+or distribution

   + const mock_paxgCollateral = BigNumber.from(1_000_000);

   + await PAXG.mint(SmartVaultManager.address, mock_paxgCollateral);

   ​

   + // Create holder1 stake

   + const tstStake1 = ethers.utils.parseEther("1000");

   + const eurosStake1 = ethers.utils.parseEther("2000");

   + await TST.mint(holder1.address, tstStake1);

   + await EUROs.mint(holder1.address, eurosStake1);

   + await TST.connect(holder1).approve(LiquidationPool.address, tstStake1);

   + await EUROs.connect(holder1).approve(

   + LiquidationPool.address,

   + eurosStake1

   + );

   + await LiquidationPool.connect(holder1).increasePosition(

   + tstStake1,

   + eurosStake1

   + );

   ​

   + await fastForward(DAY);

   ​

   + const SVM_PAXG_Balance = await PAXG.balanceOf(SmartVaultManager.address);

   + console.log("SVM_PAXG_Balance", SVM_PAXG_Balance); // 1_000_000

   ​

   + await expect(LiquidationPoolManager.runLiquidation(TOKEN_ID)).not.to.be

   + .reverted;

   ​

   + // We expect total stake to equal 1000; of which both holder1 has 100%

   + // We expect LiquidationPool's balance to be 0.02% less than SmartVaultManager

   + // because transfer of 1_000_000 between them costs 0.02%

   + const LP_PAXG_Balance = await PAXG.balanceOf(LiquidationPool.address);

   + console.log("LP_PAXG_Balance", LP_PAXG_Balance); // 999_601

   ​

   + // Expect user to get 100% of rewards

   + let { _rewards, _position } = await LiquidationPool.position(

   + holder1.address // 999_800 PAXG

   + );

   + // Expect reward assigned to holder1 to be mock_paxgCollateral+ - (mock_paxgCollateral * 0.02%)

   + expect(rewardAmountForAsset(_rewards, "PAXG")).to.equal(

   + mock_paxgCollateral - mock_paxgCollateral.div(5000)

   + );

   ​

   + // Expect actual amount sent to LP to be less than holder 1 rewards holder1 reward+ - (holder1 reward * 0.02%)

   + expect(LP_PAXG_Balance).to.equal(

   + rewardAmountForAsset(_rewards, "PAXG") -

   + rewardAmountForAsset(_rewards, "PAXG").div(5000)

   + );

   ​

   + // Expect claimRewards to revert

   + await expect(LiquidationPool.connect(holder1).claimRewards()).to.be

   + .reverted;

   + });

Tools Used

Manual Review
Hardhat Testing

Recommendations

Update distributeAssets() to check if the token is PAXG and if so apply a 0.02% reduction to the amount sent to rewards. This should be made dynamic if other fee-on-transfer tokens are going to be permitted as collateral.

```diff
    // Some Code
    _position.EUROs -= costInEuros;

+   bytes32 PAXGSymbol = keccak256(abi.encodePacked("PAXG"));
+   // Check if the token symbol is PAXG
+   if (asset.token.symbol == PAXGSymbol) {
+       uint256 fee = _portion * 2 / 10000;    // 0.02% fee
+       _portion -= fee;                       // Deduct the fee from the _portion
+   }

    rewards[abi.encodePacked(_position.holder, asset.token.sym+= _portion;
    burnEuros += costInEuros;
    if (asset.token.addr == address(0)) {
        nativePurchased += _portion;
    } else {
        // @audit-issue : 0 amount can be sent in this
        IERC20(asset.token.addr).safeTransferFrom(manager, a(this), _portion);
    }

```