Issue H-1: A malicious early user/attacker can manipulate the LToken's pricePerShare to take an unfair share of future users' deposits
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/004-H
kankodu, JohnSmith, PwnPatrol, WATCHPUG, berndartmueller, hyh, __141345__, IllIllI, TomJ
A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.
A malicious early user can deposit() with 1 wei of asset token as the first depositor of the LToken, and get 1 wei of shares.
Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .
As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token.
They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit().
The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.
function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
beforeDeposit(assets, shares);
// Check for rounding error since we round down in previewDeposit.
require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
// Need to transfer before minting or ERC777s could reenter.
asset.safeTransferFrom(msg.sender, address(this), assets);
_mint(receiver, shares);
emit Deposit(msg.sender, receiver, assets, shares);
}function previewDeposit(uint256 assets) public view virtual returns (uint256) {
return convertToShares(assets);
}function convertToShares(uint256 assets) public view virtual returns (uint256) {
uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
}function totalAssets() public view override returns (uint) {
return asset.balanceOf(address(this)) + getBorrows() - getReserves();
}Manual Review
Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the DAO so that the pricePerShare can be more resistant to manipulation.
function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
beforeDeposit(assets, shares);
// Check for rounding error since we round down in previewDeposit.
require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
// for the first mint, we require the mint amount > (10 ** decimals) / 100
// and send (10 ** decimals) / 1_000_000 of the initial supply as a reserve to DAO
if (totalSupply == 0 && decimals >= 6) {
require(shares > 10 ** (decimals - 2));
uint256 reserveShares = 10 ** (decimals - 6);
_mint(DAO, reserveShares);
shares -= reserveShares;
}
// Need to transfer before minting or ERC777s could reenter.
asset.safeTransferFrom(msg.sender, address(this), assets);
_mint(receiver, shares);
emit Deposit(msg.sender, receiver, assets, shares);
}
function mint(uint256 shares, address receiver) public virtual returns (uint256 assets) {
beforeDeposit(assets, shares);
assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
// for the first mint, we require the mint amount > (10 ** decimals) / 100
// and send (10 ** decimals) / 1_000_000 of the initial supply as a reserve to DAO
if (totalSupply == 0 && decimals >= 6) {
require(shares > 10 ** (decimals - 2));
uint256 reserveShares = 10 ** (decimals - 6);
_mint(DAO, reserveShares);
shares -= reserveShares;
}
// Need to transfer before minting or ERC777s could reenter.
asset.safeTransferFrom(msg.sender, address(this), assets);
_mint(receiver, shares);
emit Deposit(msg.sender, receiver, assets, shares);
}Fixed as recommended but instead of sending these shares to the DAO, we burn them. PR here.
Confirmed fix.
Issue H-2: ChainlinkOracle.sol#getPrice() The price will be wrong when the token's USD price feed's decimals != 8
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/019-H
Lambda, csanuragjain, CRYP70, WATCHPUG, berndartmueller, pashov, IllIllI, Bahurum, sorrynotsorry
ChainlinkOracle assumes and inexplicitly requires the token's USD feed's decimals to be 8. However, there are certain token's USD feed has a different decimals.
In the current implementation, it assumes feed[token].decimals() must equals ethUsdPriceFeed.decimals() = 8).
However, there are tokens with USD price feed's decimals != 8 (E.g: AMPL / USD feed decimals = 18)
When the token's USD feed's decimals != 8, ChainlinkOracle.sol#getPrice() will return an incorrect price in ETH.
The correct calculation formula should be:
Given:
-
1.0 AMPL worth 1.14 USD,
feed[ampl].decimals() == 18,answer_ampl = 1140608758261546000Source:feed[ampl] -
1.0 ETH worth 1588.11 USD,
ethUsdPriceFeed.decimals() == 8,answer_eth = 158811562094Source:ethUsdPriceFeed
chainlinkOracle.getPrice(AMPL) will return ~7.18m (eth):
When the price feed with decimals != 18 is set, the attacker can deposit a small amount of the asset and drain all the funds from the protocol.
function _valueInWei(address token, uint amt)
internal
view
returns (uint)
{
return oracle.getPrice(token)
.mulDivDown(
amt,
10 ** ((token == address(0)) ? 18 : IERC20(token).decimals())
);
}/// @inheritdoc IOracle
/// @dev feed[token].latestRoundData should return price scaled by 8 decimals
function getPrice(address token) external view virtual returns (uint) {
(, int answer,,,) =
feed[token].latestRoundData();
if (answer < 0)
revert Errors.NegativePrice(token, address(feed[token]));
return (
(uint(answer)*1e18)/getEthPrice()
);
}function getEthPrice() internal view returns (uint) {
(, int answer,,,) =
ethUsdPriceFeed.latestRoundData();
if (answer < 0)
revert Errors.NegativePrice(address(0), address(ethUsdPriceFeed));
return uint(answer);
}Manual Review
Consider adding a check for feed.decimals() to make sure feed's decimals = 8:
constructor(AggregatorV3Interface _ethUsdPriceFeed) Ownable(msg.sender) {
require(_ethUsdPriceFeed.decimals() == 8, "...");
ethUsdPriceFeed = _ethUsdPriceFeed;
}function setFeed(
address token,
AggregatorV3Interface _feed
) external adminOnly {
require(_feed.decimals() == 8, "...");
feed[token] = _feed;
emit UpdateFeed(token, address(_feed));
}Our chainlink oracles use token/usd feed for all the assets we support via chainlink, all the token/usd chainlink feeds have 8 decimals and hence will not lead to any issue with the final price calculation. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/021-H
0x52
CTokenOracle.sol#getCErc20Price contains a math error that immensely overvalues CTokens
function getCErc20Price(ICToken cToken, address underlying) internal view returns (uint) {
/*
cToken Exchange rates are scaled by 10^(18 - 8 + underlying token decimals) so to scale
the exchange rate to 18 decimals we must multiply it by 1e8 and then divide it by the
number of decimals in the underlying token. Finally to find the price of the cToken we
must multiply this value with the current price of the underlying token
*/
return cToken.exchangeRateStored()
.mulDivDown(1e8 , IERC20(underlying).decimals())
.mulWadDown(oracle.getPrice(underlying));
}
In L74, IERC20(underlying).decimals() is not raised to the power of 10. The results in the price of the LP being overvalued by many order of magnitudes. A user could deposit one CToken and drain the reserves of every liquidity pool.
All lenders could be drained of all their funds due to excessive over valuation of CTokens cause by this error
Manual Review
Fix the math error by changing L74:
return cToken.exchangeRateStored()
.mulDivDown(1e8 , 10 ** IERC20(underlying).decimals())
.mulWadDown(oracle.getPrice(underlying));
Fixed as recommended. PR here.
Confirmed fix.
Issue H-4: ERC4626Oracle Price will be wrong when the ERC4626's decimals is different from the underlying token’s decimals
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/025-H
Lambda, JohnSmith, WATCHPUG, 0x52, berndartmueller, Bahurum
EIP-4626 does not require the decimals must be the same as the underlying tokens' decimals, and when it's not, ERC4626Oracle will malfunction.
In the current implementation, IERC4626(token).decimals() is used as the IERC4626(token).asset()'s decimals to calculate the ERC4626's price.
However, while most ERC4626s are using the underlying token’s decimals as decimals, there are some ERC4626s use a different decimals from underlying token’s decimals since EIP-4626 does not require the decimals must be the same as the underlying token’s decimals:
Although the convertTo functions should eliminate the need for any use of an EIP-4626 Vault’s decimals variable, it is still strongly recommended to mirror the underlying token’s decimals if at all possible, to eliminate possible sources of confusion and simplify integration across front-ends and for other off-chain users.
Ref: https://eips.ethereum.org/EIPS/eip-4626
The price of ERC4626 will be significantly underestimated when the underlying token's decimals > ERC4626's decimals, and be significantly overestimated when the underlying token's decimals < ERC4626's decimals.
function getPrice(address token) external view returns (uint) {
uint decimals = IERC4626(token).decimals();
return IERC4626(token).previewRedeem(
10 ** decimals
).mulDivDown(
oracleFacade.getPrice(IERC4626(token).asset()),
10 ** decimals
);
}Manual Review
getPrice() can be changed to:
function getPrice(address token) external view returns (uint) {
uint decimals = IERC4626(token).decimals();
address underlyingToken = IERC4626(token).asset();
return IERC4626(token).previewRedeem(
10 ** decimals
).mulDivDown(
oracleFacade.getPrice(underlyingToken),
10 ** IERC20Metadata(underlyingToken).decimals()
);
}Fixed as recommended. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/026-H
Lambda, WATCHPUG, 0x52, hyh
When one of the LP token's underlying tokens decimals is not 18, the price of the LP token calculated by UniV2LPOracle will be wrong.
UniV2LPOracle is an implementation of Alpha Homora v2's Fair Uniswap's LP Token Pricing Formula:
The Formula ... of combining fair asset prices and fair asset reserves:
$$ P = 2\cdot \frac{\sqrt{r_0 \cdot r_1} \cdot \sqrt{p_0\cdot p_1}}{totalSupply}, $$
where
$r_i$ is the asset ii's pool balance and$p_i$ is the asset$i$ 's fair price.
However, the current implementation wrongful assumes
function getPrice(address pair) external view returns (uint) {
(uint r0, uint r1,) = IUniswapV2Pair(pair).getReserves();
// 2 * sqrt(r0 * r1 * p0 * p1) / totalSupply
return FixedPointMathLib.sqrt(
r0
.mulWadDown(r1)
.mulWadDown(oracle.getPrice(IUniswapV2Pair(pair).token0()))
.mulWadDown(oracle.getPrice(IUniswapV2Pair(pair).token1()))
)
.mulDivDown(2e27, IUniswapV2Pair(pair).totalSupply());
}https://github.com/transmissions11/solmate/blob/main/src/utils/FixedPointMathLib.sol
uint256 internal constant WAD = 1e18; // The scalar of ETH and most ERC20s.
function mulWadDown(uint256 x, uint256 y) internal pure returns (uint256) {
return mulDivDown(x, y, WAD); // Equivalent to (x * y) / WAD rounded down.
}https://github.com/transmissions11/solmate/blob/main/src/utils/FixedPointMathLib.sol
function mulDivDown(
uint256 x,
uint256 y,
uint256 denominator
) internal pure returns (uint256 z) {
assembly {
// Store x * y in z for now.
z := mul(x, y)
// Equivalent to require(denominator != 0 && (x == 0 || (x * y) / x == y))
if iszero(and(iszero(iszero(denominator)), or(iszero(x), eq(div(z, x), y)))) {
revert(0, 0)
}
// Divide z by the denominator.
z := div(z, denominator)
}
}When the decimals of one or both tokens in the pair is not 18, the price will be way off.
We've created a test script to demonstrate UniV2LPOracle is malfunctioning with USDC/WETH, in which USDC's decimals is 6 instead of 18.
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
import "forge-std/console.sol";
import {Test} from "forge-std/Test.sol";
import {IOracle} from "../core/IOracle.sol";
import {UniV2LpOracle} from "../uniswap/UniV2LPOracle.sol";
import {OracleFacade} from "../core/OracleFacade.sol";
import {ChainlinkOracle} from "../chainlink/ChainlinkOracle.sol";
import {WETHOracle} from "../weth/WETHOracle.sol";
import {AggregatorV3Interface} from "../chainlink/AggregatorV3Interface.sol";
import "forge-std/console.sol";
contract UniV2LPOracleTest is Test {
address constant usdc = address(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
address constant weth = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
address constant pair = address(0xB4e16d0168e52d35CaCD2c6185b44281Ec28C9Dc);
address constant ethUsdFeed =
address(0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419);
address constant usdcUsdFeed =
address(0x8fFfFfd4AfB6115b954Bd326cbe7B4BA576818f6);
ChainlinkOracle chainlinkOracle;
WETHOracle wETHOracle;
UniV2LpOracle uniV2LpOracle;
OracleFacade oracleFacade;
function setUp() public {
// core oracle
oracleFacade = new OracleFacade();
chainlinkOracle = new ChainlinkOracle(
AggregatorV3Interface(ethUsdFeed)
);
chainlinkOracle.setFeed(usdc, AggregatorV3Interface(usdcUsdFeed));
WETHOracle wethOracle = new WETHOracle();
oracleFacade.setOracle(weth, wethOracle);
oracleFacade.setOracle(usdc, chainlinkOracle);
uniV2LpOracle = new UniV2LpOracle(oracleFacade);
}
function testUniV2Price() public {
console.log(uniV2LpOracle.getPrice(pair));
}
}
reserves and totalSupply of UniswapV2 USDC/ETH Pair 0xB4e16d0168e52d35CaCD2c6185b44281Ec28C9Dc on Mainnet at the time of writing
Result of getReserves()
_reserve0 uint112 : 45456843739761
_reserve1 uint112 : 28342500764440756425363
_blockTimestampLast uint32 : 1663233599totalSupply(): 550760227054391377
// real time price
28342500764440756425363 * 2 / 550760227054391377 = 102921
// normalized r0 and r1 ✅
102728696484607347546879 / 1e18 = 102728
// current result
102728772378134052 / 1e18 = 0.10272877237813405Manual Review
Consider normalizing r0 and r1 to 18 decimals before using them in the formula.
Fixed as recommended. PRs here and here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/085-H
Lambda, Ruhum, bytehat, WATCHPUG, xiaoming90, pashov, cccz, GimelSec
Whenever the liquidity of a LToken changes, getRateFactor will be changed.
Therefore, updateState() must be called prior to the change to settle the pending interests.
Alice is a liquidity provider for LEther, Bob is a borrower.
- Alice added 1000 ETH;
- Bob borrowed 500 ETH; In which
updateState()is called. TheutilingetBorrowRatePerSecondis0.5now. - One year later (no one interacts with the
assetfor 1 year), Alice redeemed 500 ETH withredeemEth(), in whichupdateState()is not called. TheutilingetBorrowRatePerSecondis1now. - Bob called
repay(), in whichupdateState()is called to calculates the pending interest:
For Bob the borrower, the sum of principal and interest is:
But the actual sum is as below, due to updateState() is not called in redeemEth():
As a result, Bob the borrower is now paying 1375 instead of 587 for the interest, which is 2x the expected amount.
On the other hand, if another liquidity provider called depositEth() before Bob repays the loan, the actual interest can be lower than expected, which constitutes a loss of yields to Alice.
Incorrect amounts of interests will be paid by the borrowers, which can result in loss of yields to the lenders or overpaid interest for the borrowers.
function updateState() public {
if (lastUpdated == block.timestamp) return;
uint rateFactor = getRateFactor();
uint interestAccrued = borrows.mulWadUp(rateFactor);
borrows += interestAccrued;
reserves += interestAccrued.mulWadUp(reserveFactor);
lastUpdated = block.timestamp;
}
/* -------------------------------------------------------------------------- */
/* INTERNAL FUNCTIONS */
/* -------------------------------------------------------------------------- */
/**
@dev Rate Factor = Timestamp Delta * 1e18 (Scales timestamp delta to 18 decimals) * Interest Rate Per Block
Timestamp Delta = Number of seconds since last update
*/
function getRateFactor() internal view returns (uint) {
return (block.timestamp == lastUpdated) ?
0 :
((block.timestamp - lastUpdated)*1e18)
.mulWadUp(
rateModel.getBorrowRatePerSecond(
asset.balanceOf(address(this)),
borrows
)
);
}function getBorrowRatePerSecond(
uint liquidity,
uint borrows
)
external
view
returns (uint)
{
uint util = _utilization(liquidity, borrows);
return c3.mulDivDown(
(
util.mulWadDown(c1)
+ util.rpow(32, SCALE).mulWadDown(c1)
+ util.rpow(64, SCALE).mulWadDown(c2)
),
secsPerYear
);
}
function _utilization(uint liquidity, uint borrows)
internal
pure
returns (uint)
{
uint totalAssets = liquidity + borrows;
return (totalAssets == 0) ? 0 : borrows.divWadDown(totalAssets);
}updatestate must be called everytime balance of asset is changed.
https://github.com/sentimentxyz/protocol/blob/4e45871e4540df0f189f6c89deb8d34f24930120/src/tokens/LEther.sol#L26-L53
/**
@notice Wraps Eth sent by the user and deposits into the LP
Transfers shares to the user denoting the amount of Eth deposited
@dev Emits Deposit(caller, owner, assets, shares)
*/
function depositEth() external payable {
uint assets = msg.value;
uint shares = previewDeposit(assets);
require(shares != 0, "ZERO_SHARES");
IWETH(address(asset)).deposit{value: assets}();
_mint(msg.sender, shares);
emit Deposit(msg.sender, msg.sender, assets, shares);
}
/**
@notice Unwraps Eth and transfers it to the caller
Amount of Eth transferred will be the total underlying assets that
are represented by the shares
@dev Emits Withdraw(caller, receiver, owner, assets, shares);
@param shares Amount of shares to redeem
*/
function redeemEth(uint shares) external {
uint assets = previewRedeem(shares);
_burn(msg.sender, shares);
emit Withdraw(msg.sender, msg.sender, msg.sender, assets, shares);
IWETH(address(asset)).withdraw(assets);
msg.sender.safeTransferEth(assets);
}Manual Review
beforeDeposit() should be called in depositEth() and redeemEth():
/**
@notice Wraps Eth sent by the user and deposits into the LP
Transfers shares to the user denoting the amount of Eth deposited
@dev Emits Deposit(caller, owner, assets, shares)
*/
function depositEth() external payable {
uint assets = msg.value;
uint shares = previewDeposit(assets);
require(shares != 0, "ZERO_SHARES");
beforeDeposit(assets, shares);
IWETH(address(asset)).deposit{value: assets}();
_mint(msg.sender, shares);
emit Deposit(msg.sender, msg.sender, assets, shares);
}
/**
@notice Unwraps Eth and transfers it to the caller
Amount of Eth transferred will be the total underlying assets that
are represented by the shares
@dev Emits Withdraw(caller, receiver, owner, assets, shares);
@param shares Amount of shares to redeem
*/
function redeemEth(uint shares) external {
uint assets = previewRedeem(shares);
beforeWithdraw(assets, shares);
_burn(msg.sender, shares);
emit Withdraw(msg.sender, msg.sender, msg.sender, assets, shares);
IWETH(address(asset)).withdraw(assets);
msg.sender.safeTransferEth(assets);
}Fixed as recommended. PR here.
Confirmed fix.
Issue H-7: Tokens received from Curve's remove_liquidity() should be added to the assets list even if _min_amounts are set to 0
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/267-H
WATCHPUG
Curve controller's canRemoveLiquidity() should return all the underlying tokens as tokensIn rather than only the tokens with minAmount > 0.
function canRemoveLiquidity(address target, bytes calldata data)
internal
view
returns (bool, address[] memory, address[] memory)
{
(,uint256[2] memory amounts) = abi.decode(
data[4:],
(uint256, uint256[2])
);
address[] memory tokensOut = new address[](1);
tokensOut[0] = target;
uint i; uint j;
address[] memory tokensIn = new address[](2);
while(i < 2) {
if(amounts[i] > 0)
tokensIn[j++] = IStableSwapPool(target).coins(i);
unchecked { ++i; }
}
assembly { mstore(tokensIn, j) }
return (true, tokensIn, tokensOut);
}The amounts in Curve controller's canRemoveLiquidity() represent the "Minimum amounts of underlying coins to receive", which is used for slippage control.
At L144-149, only the tokens that specified a minAmount > 0 will be added to the tokensIn list, which will later be added to the account's assets list.
We believe this is wrong as regardless of the minAmount remove_liquidity() will always receive all the underlying tokens.
Therefore, it should not check and only add the token when it's minAmount > 0.
When the user set _min_amounts = 0 while removing liquidity from Curve and the withdrawn tokens are not in the account's assets list already, the user may get liquidated sooner than expected as RiskEngine.sol#_getBalance() only counts in the assets in the assets list.
https://arbiscan.io/address/0x7f90122bf0700f9e7e1f688fe926940e8839f353#code
@external
@nonreentrant('lock')
def remove_liquidity(
_burn_amount: uint256,
_min_amounts: uint256[N_COINS],
_receiver: address = msg.sender
) -> uint256[N_COINS]:
"""
@notice Withdraw coins from the pool
@dev Withdrawal amounts are based on current deposit ratios
@param _burn_amount Quantity of LP tokens to burn in the withdrawal
@param _min_amounts Minimum amounts of underlying coins to receive
@param _receiver Address that receives the withdrawn coins
@return List of amounts of coins that were withdrawn
"""
total_supply: uint256 = self.totalSupply
amounts: uint256[N_COINS] = empty(uint256[N_COINS])
for i in range(N_COINS):
old_balance: uint256 = self.balances[i]
value: uint256 = old_balance * _burn_amount / total_supply
assert value >= _min_amounts[i], "Withdrawal resulted in fewer coins than expected"
self.balances[i] = old_balance - value
amounts[i] = value
ERC20(self.coins[i]).transfer(_receiver, value)
total_supply -= _burn_amount
self.balanceOf[msg.sender] -= _burn_amount
self.totalSupply = total_supply
log Transfer(msg.sender, ZERO_ADDRESS, _burn_amount)
log RemoveLiquidity(msg.sender, amounts, empty(uint256[N_COINS]), total_supply)
return amountsManual Review
canRemoveLiquidity() can be changed to:
function canRemoveLiquidity(address target, bytes calldata data)
internal
view
returns (bool, address[] memory, address[] memory)
{
address[] memory tokensOut = new address[](1);
tokensOut[0] = target;
address[] memory tokensIn = new address[](2);
tokensIn[0] = IStableSwapPool(target).coins(0);
tokensIn[1] = IStableSwapPool(target).coins(1);
return (true, tokensIn, tokensOut);
}Fixed as recommended. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/502-H
WATCHPUG
Note: This issue is a part of the extra scope added by Sentiment AFTER the audit contest. This scope was only reviewed by WatchPug and relates to these three PRs:
Curve's gauge rewards cannot be claimed.
The sole goal of CurveLPStakingController is to stake and earn rewards from Curve's gauge system.
However, it doesn't support claim_rewards(), and both deposit() or withdraw() are using the default value (false) for _claim_rewards parameter.
Curve's gauge rewards cannot be claimed.
/// @notice deposit(uint256)
bytes4 constant DEPOSIT = 0xb6b55f25;
/// @notice withdraw(uint256)
bytes4 constant WITHDRAW = 0x2e1a7d4d;Manual Review
Consider supporting claim_rewards(address,address) or using deposit(uint256,address,bool) and withdraw(uint256,address,bool) to support claim rewards when deposit() or withdraw().
Fixed controller as recommended to claim gauge rewards and moved gauge token oracle into a separate contract instead. PR here.
Confirmed fix.
Issue M-1: Lack of price freshness check in ChainlinkOracle.sol#getPrice() allows a stale price to be used
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/002-M
defsec, icedpeachtea, oyc_109, Lambda, 0xNineDec, Avci, ladboy233, JohnSmith, jonatascm, Ruhum, csanuragjain, PwnPatrol, WATCHPUG, 0xNazgul, xiaoming90, 0x52, 0xf15ers, ellahi, pashov, rbserver, GalloDaSballo, Chom, __141345__, cccz, devtooligan, Bahurum, HonorLt, GimelSec, Dravee, Olivierdem
ChainlinkOracle should use the updatedAt value from the latestRoundData() function to make sure that the latest answer is recent enough to be used.
In the current implementation of ChainlinkOracle.sol#getPrice(), there is no freshness check. This could lead to stale prices being used.
If the market price of the token drops very quickly ("flash crashes"), and Chainlink's feed does not get updated in time, the smart contract will continue to believe the token is worth more than the market value.
Chainlink also advise developers to check for the updatedAt before using the price:
Your application should track the latestTimestamp variable or use the updatedAt value from the latestRoundData() function to make sure that the latest answer is recent enough for your application to use it. If your application detects that the reported answer is not updated within the heartbeat or within time limits that you determine are acceptable for your application, pause operation or switch to an alternate operation mode while identifying the cause of the delay.
And they have this heartbeat concept:
Chainlink Price Feeds do not provide streaming data. Rather, the aggregator updates its latestAnswer when the value deviates beyond a specified threshold or when the heartbeat idle time has passed. You can find the heartbeat and deviation values for each data feed at data.chain.link or in the Contract Addresses lists.
The Heartbeat on Arbitrum is usually 1h.
Source: https://docs.chain.link/docs/arbitrum-price-feeds/
A stale price can cause the malfunction of multiple features across the protocol:
-
_valueInWei()is using the price to calculate the value of the loan and collateral; A stale price will make the price calculation inaccurate so that certain accounts may not be liquidated when they should be, or be liquidated when they should not be. -
ChainlinkOracle.sol#getPrice()is used to calculate the value of various LPTokens (Aave, Balancer, Compound, Curve, and Uniswap). If the price is not accurate, it will lead to a deviation in the LPToken price and affect the calculation of asset prices. -
Stale asset prices can lead to bad debts to the protocol as the collateral assets can be overvalued, and the collateral value can not cover the loans.
function getPrice(address token) external view virtual returns (uint) {
(, int answer,,,) =
feed[token].latestRoundData();
if (answer < 0)
revert Errors.NegativePrice(token, address(feed[token]));
return (
(uint(answer)*1e18)/getEthPrice()
);
} function getEthPrice() internal view returns (uint) {
(, int answer,,,) =
ethUsdPriceFeed.latestRoundData();
if (answer < 0)
revert Errors.NegativePrice(address(0), address(ethUsdPriceFeed));
return uint(answer);
}Manual Review
Consider adding the missing freshness check for stale price:
function getPrice(address token) external view virtual returns (uint) {
(, int answer,,,) =
feed[token].latestRoundData();
uint validPeriod = feedValidPeriod[token];
require(block.timestamp - updatedAt < validPeriod, "freshness check failed.")
if (answer <= 0)
revert Errors.NegativePrice(token, address(feed[token]));
return (
(uint(answer)*1e18)/getEthPrice()
);
}The validPeriod can be based on the Heartbeat of the feed.
Fixed as recommended. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/029-M
Lambda, Bahurum, kirk-baird, bin2chen, xiaoming90, GalloDaSballo
Some external interactions send tokens to the account, and the token address is not checked before being registered as a collateral for the account. This allows using an arbitrary token as a collateral as long as there is a corresponding oracle.
In the following controllers, at the line referenced
UniV2Controller.sol #L189
AaveV2Controller.sol #L91
AaveV3Controller.sol #L79
BalancerController.sol#L137
There is an operation with a return value that can be an arbitrary token. No check is made to verify that the tokens in tokensIn are activated in controllerFacade.isTokenAllowed. No check is made either in AccountManager.exec() when calling _updateTokensIn() (AccountManager.sol#L305), so the tokens are added as a collateral as long as the oracle for the token in OracleFacade is active.
This can be clarified with an example using UniV2Controller.sol:
- Admin sets the oracle for token XYZ, but does not activate yet the token in
controllerFacade.isTokenAllowedor inAccountManager.isCollateralAllowedwaiting to clear some security concerns with the token (vulnerabilities, high volatility, low liquidity, ...) - Attacker opens an account
- Attacker provides liquidity to XYZ / ETH pool on UniswapV2
- Attacker transfers the LP to the account
- Attacker removes liquidity from the UniswapV2 pool through the account by calling
AccountManager.exec. Note that there is no check to control whether tokens exiting the account are allowed (UniV2Controller.sol#L175-L190). No checks are performed on tokens sent from the pool to the account, so account receives WETH + XYZ and also XYZ is registered as an account collateral (AccountManager.sol#L305). An oracle was set for XYZ so the call toriskEngine.isAccountHealthy(account)(AccountManager.sol#L310) does not revert and the transaction succeeds. - Attacker can take advantage of the aforementioned security concerns to manipulate the price of XYZ, inflate his collateral balance and drain funds from the protocol.
The same behaviour can be reproduced with the other integrations listed above by transfering LP tokens of that particular protocol directly to an account and then removing liquidity through AccountManager.exec() to get the pair tokens registered into the account as collateral.
See point 6. of section above.
UniV2Controller.sol #L189
AaveV2Controller.sol #L91
AaveV3Controller.sol #L79
BalancerController.sol#L137
AccountManager.sol#L347
Manual Review
Check if tokens are allowed as collateral in _updateTokensIn (AccountManager.sol#L347-L355)
function _updateTokensIn(address account, address[] memory tokensIn)
internal
{
uint tokensInLen = tokensIn.length;
for(uint i; i < tokensInLen; ++i) {
+ if (!isCollateralAllowed[tokensIn[i]])
+ revert Errors.CollateralTypeRestricted();
if (IAccount(account).hasAsset(tokensIn[i]) == false)
IAccount(account).addAsset(tokensIn[i]);
}
}This way no asset can be added as collateral without being allowed.
Fixed as recommended. PR here and here.
Confirmed fix. If there are non-whitelisted tokens in the tokensIn list from a controller, it seems like the system should not reject the call, but not to add these tokens (ignore these tokens).
This issue is more obvious for the case of claim rewards from 3rd party protocols, which may include non-whitelisted tokens, but that doesn't mean it should not be allowed to claim these rewards.
We intend to track rewards as well and hence will be required to add them in the tokensIn list, all rewards will be whitelisted and only those integrations will be enabled. For rewards that we don't want to use as collateral in the system, we plan to add them to the assets and point them to the zero oracle.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/033-M
panprog, csanuragjain, Czar102, carrot, PwnPatrol, Lambda, kirk-baird, berndartmueller, rbserver, Chom, __141345__
When the transfer of one asset fails, liquidations become impossible.
_liquidate calls sweepTo, which iterates over all assets. When one of those transfers fails, the whole liquidation process therefore fails. There are multiple reasons why a transfer could fail:
1.) Blocked addresses (e.g., USDC)
2.) The balance of the asset is 0, but it is still listed under asset. This can be for instance triggered by performing a 0 value Uniswap swap, in which case it is still added to tokensIn. Another way to trigger is to call deposit with amt = 0 (this is another issue that should be fixed IMO, in practice the assets of an account should not contain any tokens with zero balance)
Some tokens revert for zero value transfers (see https://github.com/d-xo/weird-erc20)
3.) Paused tokens
4.) Upgradeable tokens that changed the implementation.
See above, an account cannot be liquidated. In certain conditions, this might even be triggerable by the user. For instance, a user could try to get on the USDC blacklist to avoid liquidations.
https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/core/AccountManager.sol#L384 https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/core/Account.sol#L166
Manual Review
Catch reversions for the transfer and skip this asset (but it could be kept in the assets list to allow retries later on).
Fixed as recommended. PR here.
Confirmed fix.
Issue M-4: Accounts with ETH loans can not be liquidated if LEther's underlying is set to address(0)
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/034-M
Lambda, WATCHPUG, hansfriese, rbserver, HonorLt, 0xc0ffEE
Setting address(0) as LEther's underlying is allowed, and the logic in AccountManager#settle() and RiskEngine#_valueInWei() handles address(0) specially, which implies that address(0) can be an asset.
However, if LEther's underlying is set to address(0), the accounts with ETH loans will become unable to be liquidated.
Given that at AccountManager.sol#L100 in settle() and RiskEngine.sol#L186 in _valueInWei(), they both handled the case that the asset == address(0), and in Registry.sol#setLToken(), underlying == address(0) is allowed:
We assume that address(0) can be set as the underlying of LEther.
In that case, when the user borrows native tokens, address(0) will be added to the user's assets and borrows list.
function borrow(address account, address token, uint amt)
external
whenNotPaused
onlyOwner(account)
{
if (registry.LTokenFor(token) == address(0))
revert Errors.LTokenUnavailable();
if (!riskEngine.isBorrowAllowed(account, token, amt))
revert Errors.RiskThresholdBreached();
if (IAccount(account).hasAsset(token) == false)
IAccount(account).addAsset(token);
if (ILToken(registry.LTokenFor(token)).lendTo(account, amt))
IAccount(account).addBorrow(token);
emit Borrow(account, msg.sender, token, amt);
}This will later prevent the user from being liquidated because in riskEngine.isAccountHealthy(), it calls _getBalance() in the for loop of all the assets, which assumes all the assets complies with IERC20. Thus, the transaction will revert at L157 when calling IERC20(address(0)).balanceOf(account).
function liquidate(address account) external {
if (riskEngine.isAccountHealthy(account))
revert Errors.AccountNotLiquidatable();
_liquidate(account);
emit AccountLiquidated(account, registry.ownerFor(account));
}function _getBalance(address account) internal view returns (uint) {
address[] memory assets = IAccount(account).getAssets();
uint assetsLen = assets.length;
uint totalBalance;
for(uint i; i < assetsLen; ++i) {
totalBalance += _valueInWei(
assets[i],
IERC20(assets[i]).balanceOf(account)
);
}
return totalBalance + account.balance;
}We noticed that in the deployment documentation, LEther is set to init with WETH as the underlying. Therefore, this should not be an issue if the system is being deployed correctly.
1. ETH
1. Deploy LEther implementation
2. Deploy Proxy(LEther)
3. call init(WETH), "LEther", "LEth", IRegistry, reserveFactor)
4. call Registry.setLToken(WETH, Proxy)
5. call accountManager.toggleCollateralStatus(token)
6. call Proxy.initDep()
But considering that setting address(0) as LEther's underlying is still plausible and the potential damage to the whole protocol is high (all the accounts with ETH loans can not be liquidated), we believe that this should be a medium severity issue.
Manual Review
- Consider removing the misleading logic in
AccountManager#settle()andRiskEngine#_valueInWei()that handlesaddress(0)as an asset; - Consider disallowing adding
address(0)asunderlyinginsetLToken().
Fixed as recommended. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/035-M
Lambda, JohnSmith, PwnPatrol, IllIllI, xiaoming90, ellahi, bytehat
Rebasing tokens are tokens where balanceOf() returns larger amounts over time, due to the addition of interest to each account, or due to airdrops
Sentiment doesn't properly track balance changes while rebasing tokens are in the borrower's account
The lender will miss out on gains that should have accrued to them while the asset was lent out. While market-based price corrections may be able to handle interest that is accrued to everyone, market approaches won't work when only subsets of token addresses are given rewards, e.g. an airdrop based on a snapshot of activity that happened prior to the token being lent.
Lending tracks shares of the LToken: https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/tokens/LToken.sol#L140-L143
But repayment assumes that shares are equal to the same amount, regardless of which address held them, which is not true for airdrops: https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/tokens/LToken.sol#L160-L163
Rebasing tokens are supported, since Aave is a rebasing token: https://github.com/sherlock-audit/2022-08-sentiment/blob/main/controller/src/aave/AaveEthController.sol#L28
Manual Review
Adjust share amounts when the account balance doesn't match the share conversion calculation when taking into account gains made by the borrower
We'll make sure to not interact with fee-on-transfer tokens. This can be ensured by the admins.
Note: The admins should not add rebasing/fee-on-transfer tokens to any allowed lists.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/076-M
panprog, xiaoming90, Tutturu, devtooligan, Bahurum, Czar102
If oracle is set for any ERC777 or similar token (tokens which call receiver's hook after receiving it), re-entrancy in Account.sweepTo allows to borrow funds, which are immediately withdrawn along with all account assets without any health checks, leaving account with 0 assets and big debt, making it possible to drain all LToken funds.
If oracle is set for ERC777 token, it's possible to add these tokens to assets (even if collateral or controller allowed token for them is not set, using the bug in uniswap v2 controller for removeLiquidity, which doesn't check if tokenIn is allowed).
Once ERC777 token is added to assets, malicious user can close his account, which calls Account.sweepTo, which has multiple re-entrancy scenarios. As ERC777 token will call user's hook after receiving these tokens, the following actions are possible in the hook to steal funds:
- User can borrow funds, exit reentrancy, and all borrowed tokens along with all account assets will be immediately transfered to user without any health checks.
- For tokens which were in
assetslist before theERC777token,hasAsset[]is set to false even though the token is still in the assets list. Depositing these tokens again will add them to assets again, counting them twice in account balance calculations, which allows to borrow even more funds. - Ether is transferred back to user after all the
ERC20tokens, so ether can also be used to borrow funds against, and it will also be transerred back to user along with all theERC20tokens.
List of bugs used in the attack:
- Uniswap v2 controller in
removeLiquiditydoesn't check if tokens received are allowed. It seems to assume that account can only have allowed uni v2 lp tokens (as it's checked inaddLiquidity), however any lp tokens can easily be transferred to account directly (not viaexec). This makes it possible to callexectoremoveLiquidityand add ANY tokens to account's assets list (and if oracle is set for the token,accountManager.execwill succeed as it doesn't check for asset tokens to be allowed as collateral)
Account.sweepTois very vulnerable to re-entrancy. While it assumes that asset tokens are ERC20, if ERC20-compatible tokens with callback hooks are allowed (such asERC777tokens), these tokens can re-enter to steal funds.
Steps to steal funds from LToken:
- OpenAccount
- If
ERC777token is allowed as collateral, depositERC777token (such asimBTC, which is pegged 1:1 to BTC and is ERC777). IfERC777is not allowed as collateral, but the oracle for this token is set up, then use bug 1: 2.1. Deploy uniswap v2 poolUSDC-imBTC, add liquidity with minimumUSDCandimBTC2.2. Send uniswap v2 LP token to account's address 2.3. CallAccountManager.exectoremoveLiquidityfrom uniswap v2 LP token, abusing bug 1 and receiving small amounts ofUSDC+imBTC(tokensInare set and bothUSDCandimBTCare added to account'sassets) - Deposit
0 DAIto addDAIto accountassets(this is to includeDAIinsweepToat the last index, which will make it possible to borrowDAIin re-entrancy and immediately receive all of it without health checks) - Close account
- Close account will performs
account.sweepToas the last step, whereUSDCis transferred to user, thenimBTCis transferred to user, which callstokensReceivedon user's contract allowing re-entrancy. IntokensReceived: 5.1. CallAccountManager.openAccountto re-gain access to account (since it's closed) - this restores account access 5.2. Deposit1 etherto account 5.3. Borrow4000 DAI5.4. Exit re-entrancy sweepTocontinues and sends4000 DAIto user, then deletesassetsand sends1 etherto user.- At this point user has received back its
1 etherand4000 DAI, leaving account with0assets and4000 DAIdebt.
This scenario can be made even more capital efficient to steal more money if user deposits 1000 USDC instead of 1 ether, which will add USDC to assets, making his balance 2000 instead of 1000, allowing to borrow 8000 DAI for the same deposited amount. If this is repeated a few times, it's possible to steal millions with only a small initial capital.
If any ERC777 (or similar) token oracle is added, it's very easy to steal all funds from all LTokens which you can borrow from.
Create folder ./protocol/src/test/integrations/attacks and put these 2 test files there:
https://gist.github.com/panprog/2f16348325303869f7d84653cf99fba1
Manual Review
- Add ReEntrancy guard from openzeppelin to main entry functions, mostly all functions in
AccountManager. - Add correct token checks to uniswap v2 controller.
We found an additional issue reported by a community member which was not a real exploit but exploited a vulnerability in the logic. Would request the auditors to take a look. PR here.
Fixed.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/092-M
PwnPatrol
In the ArbiDeploymentFlow.md doc, it specifies the plan to add the UniV3Controller to controllerFacade, and then update it so it applies to Router (address: 0xE592427A0AEce92De3Edee1F18E0157C05861564).
However, some of the function signatures of the Router at this address don't match the signatures specified in the controller, so function calls will revert.
In UniV3Controller.sol, the function signatures that are allowed to be used when calling the contract are specified.
However, these signatures differ between the two Uniswap V3 router contracts due to a change in the parameters for the arguments (in all four functions, the original router included a uint256 for duration, and the Router 2 contract does not).
As a result, the function signatures in the controller do not line up with the signatures in the Router contract that is specified in the deployment doc:
- exactInputSingle should be 0x414bf389, not 0x04e45aaf
- exactOutputSingle should be 0xdb3e2198, not 0x5023b4df
- exactInput should be 0xc04b8d59, not 0xb858183f
- exactOutput should be 0xf28c0498, not 0x09b81346
Users attempting to interact with Uniswap V3 through the UniV3Controller will have their calls rejected due to mismatched function signatures.
/// @notice exactInputSingle((address,address,uint24,address,uint256,uint256,uint160)) function signature
bytes4 constant EXACT_INPUT_SINGLE = 0x04e45aaf;
/// @notice exactOutputSingle((address,address,uint24,address,uint256,uint256,uint160)) function signature
bytes4 constant EXACT_OUTPUT_SINGLE = 0x5023b4df;
/// @notice exactInput((bytes,address,uint256,uint256)) function signature
bytes4 constant EXACT_INPUT = 0xb858183f;
/// @notice exactOutput((bytes,address,uint256,uint256)) function signature
bytes4 constant EXACT_OUTPUT = 0x09b81346;Manual Review
Use Router 2 (address: 0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45) when calling controllerFacade.updateController() to connect UniV3Controller to the correct router.
Alternatively, add the extra function signatures in UniV3Controller.sol so the controller is able to work on either Router.
Fixed as recommended. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/097-M
PwnPatrol
Missing revert keyword in functionDelegateCall bypasses an intended safety check, allowing the function to fail silently.
In the helper function functionDelegateCall, there is a check to confirm that the target being called is a contract.
if (!isContract(target)) Errors.AddressNotContract;However, there is a typo in the check that is missing the revert keyword.
As a result, non-contracts can be submitted as targets, which will cause the delegatecall below to return success (because EVM treats no code as STOP opcode), even though it doesn't do anything.
(bool success, ) = target.delegatecall(data);
require(success, "CALL_FAILED");The code doesn't accomplish its intended goal of checking to confirm that only contracts are passed as targets, so delegatecalls can silently fail.
https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/utils/Helpers.sol#L66-L73
function functionDelegateCall(
address target,
bytes calldata data
) internal {
if (!isContract(target)) Errors.AddressNotContract;
(bool success, ) = target.delegatecall(data);
require(success, "CALL_FAILED");
}Manual Review
Add missing revert keyword to L70 of Helpers.sol.
if (!isContract(target)) revert Errors.AddressNotContract;Fixed as recommended. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/122-M
xiaoming90
Protocol reserve, which serves as a liquidity backstop or to compensate the protocol, within a LToken vault can be lent out to the borrowers.
The purpose of the protocol reserve within a LToken vault is to compensate the protocol or serve as a liquidity backstop. However, based on the current setup, it is possible for the protocol reserve within a Ltoken vault to be lent out.
The following functions within the LToken contract show that the protocol reserve is intentionally preserved by removing the protocol reserve from the calculation of total assets within a LToken vault. As such, whenever the Liquidity Providers (LPs) attempt to redeem their LP token, the protocol reserves will stay intact and will not be withdrawn by the LPs.
https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/tokens/LToken.sol#L191
function totalAssets() public view override returns (uint) {
return asset.balanceOf(address(this)) + getBorrows() - getReserves();
}https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/tokens/LToken.sol#L195
function getBorrows() public view returns (uint) {
return borrows + borrows.mulWadUp(getRateFactor());
}https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/tokens/LToken.sol#L176
function getReserves() public view returns (uint) {
return reserves + borrows.mulWadUp(getRateFactor())
.mulWadUp(reserveFactor);
}However, this measure is not applied consistently across the protocol. The following lendTo function shows that as long as the borrower has sufficient collateral to ensure their account remains healthy, the borrower could borrow as many assets from the LToken vault as they wish.
In the worst-case scenario, the borrower can borrow all the assets from the LToken vault, including the protocol reserve.
https://github.com/sherlock-audit/2022-08-sentiment/blob/main/protocol/src/tokens/LToken.sol#L128
File: LToken.sol
121: /**
122: @notice Lends a specified amount of underlying asset to an account
123: @param account Address of account
124: @param amt Amount of token to lend
125: @return isFirstBorrow Returns if the account is borrowing the asset for
126: the first time
127: */
128: function lendTo(address account, uint amt)
129: external
130: whenNotPaused
131: accountManagerOnly
132: returns (bool isFirstBorrow)
133: {
134: updateState();
135: isFirstBorrow = (borrowsOf[account] == 0);
136:
137: uint borrowShares;
138: require((borrowShares = convertAssetToBorrowShares(amt)) != 0, "ZERO_BORROW_SHARES");
139: totalBorrowShares += borrowShares;
140: borrowsOf[account] += borrowShares;
141:
142: borrows += amt;
143: asset.safeTransfer(account, amt);
144: return isFirstBorrow;
145: }The purpose of the protocol reserve within a LToken vault is to compensate the protocol or serve as a liquidity backstop. Without the protocol reserve, the protocol will become illiquidity, and there is no fund to compensate the protocol.
Consider updating the lendTo function to ensure that the protocol reserve is preserved and cannot be lent out. If the underlying asset of a LToken vault is less than or equal to the protocol reserve, the lending should be paused as it is more important to preserve the protocol reserve compared to lending them out.
function lendTo(address account, uint amt)
external
whenNotPaused
accountManagerOnly
returns (bool isFirstBorrow)
{
updateState();
isFirstBorrow = (borrowsOf[account] == 0);
require
uint borrowShares;
require((borrowShares = convertAssetToBorrowShares(amt)) != 0, "ZERO_BORROW_SHARES");
totalBorrowShares += borrowShares;
borrowsOf[account] += borrowShares;
borrows += amt;
asset.safeTransfer(account, amt);
+ require(asset.balanceOf(address(this)) >= getReserves(), "Not enough liquidity for lending")
return isFirstBorrow;
}We removed reserves completely in this PR.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/133-M
xiaoming90, IllIllI
ERC4626 oracle is vulnerable to price manipulation. This allows an attacker to increase or decrease the price to carry out various attacks against the protocol.
The getPrice function within the ERC4626Oracle contract is vulnerable to price manipulation because the price can be increased or decreased within a single transaction/block.
Based on the getPrice function, the price of the LP token of an ERC4626 vault is dependent on the ERC4626.previewRedeem and oracleFacade.getPrice functions. If the value returns by either ERC4626.previewRedeem or oracleFacade.getPrice can be manipulated within a single transaction/block, the price of the LP token of an ERC4626 vault is considered to be vulnerable to price manipulation.
File: ERC4626Oracle.sol
35: function getPrice(address token) external view returns (uint) {
36: uint decimals = IERC4626(token).decimals();
37: return IERC4626(token).previewRedeem(
38: 10 ** decimals
39: ).mulDivDown(
40: oracleFacade.getPrice(IERC4626(token).asset()),
41: 10 ** decimals
42: );
43: }It was observed that the ERC4626.previewRedeem couldbe manipulated within a single transaction/block. As shown below, the previewRedeem function will call the convertToAssets function. Within the convertToAssets, the number of assets per share is calculated based on the current/spot total assets and current/spot supply that can be increased or decreased within a single block/transaction by calling the vault's deposit, mint, withdraw or redeem functions. This allows the attacker to artificially inflate or deflate the price within a single block/transaction.
File: ERC4626.sol
154: function previewRedeem(uint256 shares) public view virtual returns (uint256) {
155: return convertToAssets(shares);
156: }File: ERC4626.sol
132: function convertToAssets(uint256 shares) public view virtual returns (uint256) {
133: uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
134:
135: return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
136: }The attacker could perform price manipulation to make the apparent value of an asset to be much higher or much lower than the true value of the asset. Following are some risks of price manipulation:
- An attacker can increase the value of their collaterals to increase their borrowing power so that they can borrow more assets than they are allowed from Sentiment.
- An attacker can decrease the value of some collaterals and attempt to liquidate another user account prematurely.
Avoid using previewRedeem function to calculate the price of the LP token of an ERC4626 vault. Consider implementing TWAP so that the price cannot be inflated or deflated within a single block/transaction or within a short period of time.
Depends on the integration itself, so there's no action that can be taken right now.
Acknowledged.
Issue M-11: Delisted assets can still be deposited and borrowed against by accounts that already have them
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/154-M
0x52, Kumpa, devtooligan, WATCHPUG
Delisting an asset does not prevent accounts, that already contain the asset from depositing more. It blocks the deposit function but users can sidestep but sending tokens directly to their account
AccountManger.sol#deposit attempts to block deposits from assets that are not on the list of supported collateral. When calculating the health of the account, the total balance of the account address is considered. An account that already has the asset on it's assets list doesn't need to use AccountManger.sol#deposit because they can transfer the tokens directly to the account contract. This means that these account can continue to add to and use delisted assets as collateral.
One of two scenarios depending on actions taken by the protocol. If the asset is delisted from accountManager.sol and the oracle is removed then all users with loans taken against the asset will likely be immediately liquidated, which is highly unfair to users. If the asset is just delisted from accountManager.sol then existing users that already have the asset would be able to continue using the asset to take loans. If the reason an asset is being delisted is to prevent a vulnerability then the exploit would still be able to happen due to these accounts sidestepping deposit restrictions.
Manual Review
Calculations for account health should be split into two distinct categories. When calculating the health of a position post-action, unsupported assets should not be considered in the total value of the account. When calculating the health of a position for liquidation, all assets on the asset list should be considered. This prevent any new borrowing against a delisted asset but doesn't risk all affected users being liquidated unfairly.
Fixed as recommended. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/204-M
GalloDaSballo
Yearn vaults use multiple strategies, some of them can incurr a loss, the loss will be charged to the caller when withdrawing (the caller being the protocol).
The Ytoken.getPrice incorrectly assumes that a withdrawal of 10 ** decimals is equivalent to a bigger withdrawal.
Because of the Yearn V2 codebase, we can see that a loss may happen on withdraw: https://github.com/yearn/yearn-vaults/blob/efb47d8a84fcb13ceebd3ceb11b126b323bcc05d/contracts/Vault.vy#L1074
Meaning that using the 10 ** decimals price for quoting is incorrect as it doesn't represent the total liquidatable assets, which may be very different (especially for tokens that use multiple strategies, especially levered strategies such as Levered AAVE, Levered COMP etc..)
- Attacker knows vault can only withdraw up to 50% of the underlying before getting rekt
- Attacker borrows 100% of token value, the oracle allows this as the oracle only checks for the price of 10 ** decimals tokens
- Attacker purposefully get's liquidated
- The system withdraws from the yearn vault and incurs a loss
- Attacker has ran away with more value than intended
- Additionally, self-liquidation may be used as if the Yearn Vault is using any liquidity pool to liquidate to token, the imbalance caused by the withdrawal could itself create an opportunity for an arbitrage
Oracle will allow to borrow more than what is liquidatable from the yVault
Manual Review
Refactor getPrice(address) to getPrice(address, amount) and pass in the exact amount to ensure it can be liquidated fully without a loss
Alternatively cap the Yearn tokens to a massively small LTV (35% is probably as high as you should go without simming on a token by token basis)
Not fixing since we don't plan to launch with Yearn. We are okay with not including this contract in the coverage scope since it won't be deployed.
Acknowledged.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/208-M
GalloDaSballo
The YearnController is using:
/// @notice withdraw(uint256) function signature
bytes4 constant WITHDRAW = 0x2e1a7d4d;
Which is the selector for withdraw(amount, msg.sender, 1) where the max_loss is 1bps
See Yearn Code: https://github.com/yearn/yearn-vaults/blob/efb47d8a84fcb13ceebd3ceb11b126b323bcc05d/contracts/Vault.vy#L1010
On one hand the selector will allow for loss, if you'd want to have no loss you should change it to a function call with a hardcoded 0 value for maxLoss
On the other hand, if the account has too big of a position in a Yearn Vault (e.g. more than 10% of the vault), then a withdrawal may simply not be possible as the function will always revert.
You can monitor the limits on https://yearn.watch/, per the Yearn withdraw code, the function will go through the withdrawal queue, adding up losses (if any) to the caller.
Because the hardcoded selector will not offer any flexibility to the maxLoss parameter, in any scenario in which the losses are above 1 bps, the function will simply revert, preventing any withdrawal.
Manual Review
Consider adding support for a more complete withdraw that allows to change the maxLoss parameter
Not fixing since we don't plan to launch with Yearn. We are okay with not including this contract in the coverage scope since it won't be deployed.
Acknowledged.
Issue M-14: Reserves should not be considered part of the available liquidity while calculating the interest rate
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/266-M
WATCHPUG
The implementation is different from the documentation regarding the interest rate formula.
The formula given in the docs:
Calculates Borrow rate per second:
$$ Borrow Rate Per Second = c3 \cdot (util \cdot c1 + util^{32} \cdot c1 + util^{64} \cdot c2) \div secsPerYear $$
where,
$util = borrows \div (liquidity - reserves + borrows)$
$$ util=borrows \div (liquidity−reserves+borrows) $$
function getRateFactor() internal view returns (uint) {
return (block.timestamp == lastUpdated) ?
0 :
((block.timestamp - lastUpdated)*1e18)
.mulWadUp(
rateModel.getBorrowRatePerSecond(
asset.balanceOf(address(this)),
borrows
)
);
}However, the current implementation is taking all the balance as the liquidity:
function getBorrowRatePerSecond(
uint liquidity,
uint borrows
)
external
view
returns (uint)
{
uint util = _utilization(liquidity, borrows);
return c3.mulDivDown(
(
util.mulWadDown(c1)
+ util.rpow(32, SCALE).mulWadDown(c1)
+ util.rpow(64, SCALE).mulWadDown(c2)
),
secsPerYear
);
} function _utilization(uint liquidity, uint borrows)
internal
pure
returns (uint)
{
uint totalAssets = liquidity + borrows;
return (totalAssets == 0) ? 0 : borrows.divWadDown(totalAssets);
}Per the docs, when calculating the interest rate, util is the ratio of available liquidity to the borrows, available liquidity should not include reserves.
The current implementation is using all the balance as the liquidity, this will make the interest rate lower than expectation.
Given:
asset.address(this) + borrows = 10000reserves = 1500, borrows = 7000
Expected result:
When calculating getRateFactor(), available liquidity should be asset.balanceOf(address(this)) - reserves = 1500, util = 7000 / 8500 = 0.82, getBorrowRatePerSecond() = 9114134329
Actual result:
When calculating getRateFactor(), asset.balanceOf(address(this)) = 3000, util = 0.7e18, getBorrowRatePerSecond() = 7763863430
The actual interest rate is only 7763863430 / 9114134329 = 85% of the expected rate.
Manual Review
The implementation of getRateFactor() can be updated to:
function getRateFactor() internal view returns (uint) {
return (block.timestamp == lastUpdated) ?
0 :
((block.timestamp - lastUpdated)*1e18)
.mulWadUp(
rateModel.getBorrowRatePerSecond(
asset.balanceOf(address(this)) - reserves,
borrows
)
);
}Removed reserves from LToken and added an alternate mechanism to collect direct fees.
originationFee may result in the borrower account becoming liquidatable immediately (aka WP-M2).
Fixed as recommended. PR here.
riskEngine.isBorrowAllowed should be removed as it's no longer needed.
Pushed a commit to remove the redundant call to riskEngine. PR here.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/500-M
WATCHPUG
Note: This issue is a part of the extra scope added by Sentiment AFTER the audit contest. This scope was only reviewed by WatchPug and relates to these three PRs:
LToken's implmentation is not fully up to EIP-4626's specification. This issue is would actually be considered a Low issue if it were a part of a Sherlock contest.
function maxMint(address) public view virtual returns (uint256) {
return type(uint256).max;
}MUST return the maximum amount of shares mint would allow to be deposited to receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary). This assumes that the user has infinite assets, i.e. MUST NOT rely on balanceOf of asset.
maxMint() and maxDeposit() should reflect the limitation of maxSupply.
Could cause unexpected behavior in the future due to non-compliance with EIP-4626 standard.
function maxMint(address) public view virtual returns (uint256) {
return type(uint256).max;
}Manual Review
maxMint() and maxDeposit() should reflect the limitation of maxSupply.
Consider changing maxMint() and maxDeposit() to:
function maxMint(address) public view virtual returns (uint256) {
if (totalSupply >= maxSupply) {
return 0;
}
return maxSupply - totalSupply;
}function maxDeposit(address) public view virtual returns (uint256) {
return convertToAssets(maxMint(address(0)));
}Fixed as recommended. PR here.
Confirmed fix.
Source: https://github.com/sherlock-audit/2022-08-sentiment-judging/tree/main/501-M
WATCHPUG
Note: This issue is a part of the extra scope added by Sentiment AFTER the audit contest. This scope was only reviewed by WatchPug and relates to these three PRs:
originationFee may result in the borrower account becoming liquidatable immediately.
When checking riskEngine.isBorrowAllowed(), the originationFee of the borrow is not considered. Thus, when originationFee is large enough, the borrower account becomes liquidatable immediately.
For example:
Let's say USDC's originationFee is 30%;
Alice has 100 USDC, and 0 debt in her account;
Alice borrowed 400 USDC, received only 280 USDC after the originationFee;
Alice's account is now liquidatable.
Actually, in the case above, Alice's account won't even get liquidated as all the assets are worth (380 USDC) less than the total debt (400 USDC).
originationFee may result in the borrower account becoming liquidatable immediately.
function updateOriginationFee(uint _originationFee) external adminOnly {
originationFee = _originationFee;
}function borrow(address account, address token, uint amt)
external
whenNotPaused
onlyOwner(account)
{
if (registry.LTokenFor(token) == address(0))
revert Errors.LTokenUnavailable();
if (!riskEngine.isBorrowAllowed(account, token, amt))
revert Errors.RiskThresholdBreached();
if (IAccount(account).hasAsset(token) == false)
IAccount(account).addAsset(token);
if (ILToken(registry.LTokenFor(token)).lendTo(account, amt))
IAccount(account).addBorrow(token);
emit Borrow(account, msg.sender, token, amt);
}function isBorrowAllowed(
address account,
address token,
uint amt
)
external
view
returns (bool)
{
uint borrowValue = _valueInWei(token, amt);
return _isAccountHealthy(
_getBalance(account) + borrowValue,
_getBorrows(account) + borrowValue
);
}Manual Review
Consider asserting riskEngine.isAccountHealthy() after the borrow:
function borrow(address account, address token, uint amt)
external
whenNotPaused
onlyOwner(account)
{
if (registry.LTokenFor(token) == address(0))
revert Errors.LTokenUnavailable();
if (IAccount(account).hasAsset(token) == false)
IAccount(account).addAsset(token);
if (ILToken(registry.LTokenFor(token)).lendTo(account, amt))
IAccount(account).addBorrow(token);
if (!riskEngine.isAccountHealthy(account))
revert Errors.RiskThresholdBreached();
emit Borrow(account, msg.sender, token, amt);
}Fixed as recommended. PR here.
riskEngine.isBorrowAllowed should be removed as it's no longer needed.
Pushed a commit to remove the redundant call to riskEngine, you can find it here.