The Standard

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

Users can infinitely increase their rewards by calling the `distributeAssets()` function with arbitrary parameters

Impact

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L205-L241

By calling the distributeAssets() function with specific parameters, a malicious actor can increase its rewards as much as he wants before claiming them using the claimRewards() function.

As a result, the total amount of assets accrued from the fees and the vault liquidations can be stolen by the attacker.

Proof of concept

Assume Eve and 9 other users have staked the following in the LiquidationPool contract :

  • 100 TST

  • 100 EUROs

Eve owns 10% of the stakeTotal.

After 1 day, her position is now eligible to receive rewards as her pendingStakes have passed the required deadline specified in the consolidatePendingStakes() function.

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L119-L132

function consolidatePendingStakes() private {
uint256 deadline = block.timestamp - 1 days;
for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
PendingStake memory _stake = pendingStakes[uint256(i)];
if (_stake.createdAt < deadline) {
positions[_stake.holder].holder = _stake.holder;
positions[_stake.holder].TST += _stake.TST;
positions[_stake.holder].EUROs += _stake.EUROs;
deletePendingStake(uint256(i));
// pause iterating on loop because there has been a deletion. "next" item has same index
i--;
}
}
}

Assume the LiquidationPool contract has accumulated the following through vaults liquidation :

  • 10 WBTC (this can be any ERC20 token allowed by the protocol)

Eve wants to increase her rewards for the WBTC token and crafts a specific set of parameters for the distributeAssets() function before executing it :

ILiquidationPoolManager.Asset[] memory maliciousAssets = new ILiquidationPoolManager.Asset[](1);
maliciousAssets[0] = ILiquidationPoolManager.Asset({
ITokenManager.Token : ITokenManager.Token({
symbol : "WBTC",
addr : ADDRESS_OF_WBTC,
dec : DECIMALS_OF_WBTC,
clAddr : chainlinkAddr,
clDec : chainlinkDec
}),
amount : 10 ** DECIMALS_OF_WBTC
});
uint256 maliciousCollateralRate = 1e70; // this needs to be huge
uint256 maliciousHundredPC = 1;
distributeAssets(maliciousAssets, maliciousCollateralRate, maliciousHundredPC);

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L205-L241

The distributeAssets() function will first calculate the stakeTotal variable using the getStakeTotal() function which will equal 1000.

After that, the function will iterate through every single holder (meaning they will also benefit from the vulnerability in some way) and set the _positionStake to 100 since every of the 10 holders (including Eve) holds the same amount of tokens.

Then, it will iterate (only once) through the malicious _asset array, cache the asset variable and set the _portion as follows

uint256 _portion = asset.amount * _positionStake / stakeTotal;
// _portion will equal 1 WBTC

Now, the costInEuros is calculated and is meant to be subtracted from the holder's position BUT because Eve supplied an enormous uint256 as _collateralRate, the calculation will return 0. This means, nothing will be subtracted from the holder's position. Moreover, this costInEuros is supposed to be used to burn some EUROs token right after, which won't happen.

Here is a PoC in chisel that will calculate the costInEuros variable with the malicious parameters using fictive (but close to reality) WBTC/USD and USD/EUR prices

function costInEuros(uint256 collateralRate, uint256 hundredPC) public returns(uint256){
return 1e18 * 10 ** (18 - 18) * 44000 / 1 * hundredPC / collateralRate;
}
➜ uint256 cost = costInEuros(1e70,1);
➜ cost
Type: uint
Hex: 0x0
Decimal: 0

Finally, the rewards will be increased by the _portion (1 WBTC) and the amount of WBTC will be transferred from the LiquidationPoolManager to the LiquidationPool.

Eve can now execute the exact same transaction 9 more times (for a total of 10 times) to increase her rewards up to 10 WBTC.

Since her rewards for WBTC have now reached 10 WBTC, if she calls the claimRewards() function, 10 WBTC will be transferred to her, leaving nothing for the other holders.

Note that any of the other holders could have called claimRewards() as well in order to grab the 10 WBTC

Tools used

  • Manual analysis

  • Foundry (chisel)

Recommended mitigation steps

Depending on the protocol, here are 2 possibilities :

  1. Add access control to the distributeAssets() function to allow only trusted parties to execute it (e.g. the DAO).

  2. Calculate the _collateralRate and _hundredPC internally instead of providing them as function parameters

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!