Submission Details
Severity:
Can't liquidate tokens that revert on `approve` from an existing non-zero allowance
Summary
Some ERC20 tokens do not work when changing the allowance from an existing non-zero allowance value
Vulnerability Details
Some ERC20 tokens do not work when changing the allowance from an existing non-zero allowance value. For example Tether (USDT)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals.
https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPoolManager.sol#L76
ierc20.approve(pool, erc20balance);
In distributeAssets we do division, which will often loose precision for dust values, and not all the approval will be used. It means that the second call to runLiquidation will revert (see PoC)
https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L259
uint256 _portion = asset.amount * _positionStake / stakeTotal;
Impact
Can liquidate more than once (different Vaults) for the same token. It will lead to bad debt
Proof of Concept
Put solidity file in contracts/utils/. Put the js file in test
// SPDX-License-Identifier: UNLICENSED
import "./ERC20Mock.sol";
contract USDTApprovalMock is ERC20 {
constructor(string memory _name, string memory _symbol) ERC20(_name, _symbol) {
}
function mint(address to, uint256 amount) public {
_mint(to, amount);
}
// To simplify I will return true, even so the real one does not
// It doesn't matter for current example
function approve(address _spender, uint _value) public override returns (bool) {
// https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L205
// Changed but the meaning is kept
require(!((_value != 0) && (allowance(msg.sender, _spender) != 0)), 'Failed in USDTApprovalMock');
return super.approve(_spender, _value);
}
}
// @ts-check
const { expect } = require("chai");
const { ethers, network } = require("hardhat");
const { BigNumber } = ethers;
const { mockTokenManager, DEFAULT_EUR_USD_PRICE, DEFAULT_ETH_USD_PRICE, DEFAULT_WBTC_USD_PRICE, DEFAULT_USDC_USD_PRICE, DEFAULT_COLLATERAL_RATE, HUNDRED_PC, TOKEN_ID, rewardAmountForAsset, fastForward, DAY, POOL_FEE_PERCENTAGE, fullyUpgradedSmartVaultManager, PROTOCOL_FEE_RATE } = require("./common");
describe('LiquidationPoolManager', async () => {
let LiquidationPoolManager, LiquidationPoolManagerContract, LiquidationPool, SmartVaultManager, TokenManager,
TST, EUROs, WBTC, USDC, holder1, holder2, holder3, holder4, holder5, Protocol, ERC20MockFactory;
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());
SmartVaultManager = await (await ethers.getContractFactory('MockSmartVaultManager')).deploy(DEFAULT_COLLATERAL_RATE, TokenManager.address);
const EurUsd = await (await ethers.getContractFactory('ChainlinkMock')).deploy('EUR/USD'); // $1.06
await EurUsd.setPrice(DEFAULT_EUR_USD_PRICE);
LiquidationPoolManagerContract = await ethers.getContractFactory('LiquidationPoolManager');
LiquidationPoolManager = await LiquidationPoolManagerContract.deploy(
TST.address, EUROs.address, SmartVaultManager.address, EurUsd.address, Protocol.address, POOL_FEE_PERCENTAGE
);
LiquidationPool = await ethers.getContractAt('LiquidationPool', await LiquidationPoolManager.pool());
await EUROs.grantRole(await EUROs.BURNER_ROLE(), LiquidationPool.address);
});
afterEach(async () => {
await network.provider.send("hardhat_reset")
});
describe('runLiquidation', async () => {
it('forwards fees and rewards to protocol if there is no tst staked in pool', async () => {
const fees = ethers.utils.parseEther('1000');
const USDT = await prepareUSDT();
// 2 stakers, because of rounding errors in `distributeAssets` it will lead to non-zero allowance left
/*
uint256 _portion = asset.amount * _positionStake / stakeTotal;
console.log(asset.amount); console.log(_portion);
Output:
100000000
99900099
100000000
99900
*/
await stake(holder1, ethers.utils.parseEther('1000'));
await stake(holder2, ethers.utils.parseEther('1'));
await EUROs.mint(LiquidationPoolManager.address, fees);
console.log(
'allowance before liquidation',
await USDT.allowance(LiquidationPoolManager.address, LiquidationPool.address)
);
await LiquidationPoolManager.runLiquidation(TOKEN_ID);
const allowanceAfter = await USDT
.allowance(LiquidationPoolManager.address, LiquidationPool.address);
console.log( 'allowance after liquidation', allowanceAfter );
expect(allowanceAfter).to.equal(BigNumber.from('1'));
await USDT.mint(SmartVaultManager.address, BigNumber.from(100000000));
try {
await LiquidationPoolManager.runLiquidation(TOKEN_ID);
expect.fail('Transaction should have failed but did not');
} catch (error) {
console.log(error.message);
expect(error.message).to.include(
'Failed in USDTApprovalMock',
'Transaction failed for an unexpected reason'
);
}
});
async function stake(holder, stake) {
await EUROs.mint(holder.address, stake);
await EUROs.connect(holder).approve(LiquidationPool.address, stake);
await TST.mint(holder.address, stake);
await TST.connect(holder).approve(LiquidationPool.address, stake);
await LiquidationPool.connect(holder).increasePosition(stake, stake);
fastForward(DAY);
}
async function prepareUSDT() {
const usdtCollateral = BigNumber.from(100000000);
const UsdtFactory = await ethers.getContractFactory('USDTApprovalMock');
const USDT = await UsdtFactory.deploy('USDT', 'USDT');
const UsdtUsd = await (await ethers.getContractFactory('ChainlinkMock')).deploy('USDT/USD'); // 1$
await UsdtUsd.setPrice(DEFAULT_USDC_USD_PRICE);
await TokenManager.addAcceptedToken(USDT.address, UsdtUsd.address);
await USDT.mint(SmartVaultManager.address, usdtCollateral);
return USDT;
}
});
});
Tools Used
Manual review
Recommended Mitigation Steps
Set approval to 0 first
Updates
Lead Judging Commences
hrishibhat over 1 year ago
Submission Judgement Published Reason: Out of scope
Assigned finding tags:
pool-approval
informational/invalid
Prize pool breakdown
Total prize
20,000 USDC
nSLOC
60933 USDC / LOC
17,500 USDC
2,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.