| sponsor | slug | date | title | findings | contest |
|---|---|---|---|---|---|
NOYA |
2024-04-noya |
2025-02-17 |
NOYA |
368 |
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the NOYA smart contract system written in Solidity. The audit took place between April 26—May 17 2024.
173 Wardens contributed reports to NOYA:
- BenRai
- 0xAlix2 (a_kalout and ali_shehab)
- alexxander
- Sentryx (flacko and Buggsy)
- KYP (givn and yotov721)
- pkqs90
- grearlake
- iamandreiski
- SeveritySquad (shealtielanz and 0xdice91)
- joaovwfreire
- KupiaSec
- said
- rbserver
- MrPotatoMagic
- VAD37
- SpicyMeatball
- Bauchibred
- ArsenLupin
- ladboy233
- ZanyBonzy
- WinSec (ni8mare, 0xanmol, and 0xweb3boy)
- AlexCzm
- SBSecurity (Slavcheww and Blckhv)
- Weed0607
- oakcobalt
- bigtone
- Tigerfrake
- d3e4
- DPS (yvuchev and 0xJaeger)
- MSaptarshi
- Rhaydden
- Inspex (DeStinE21, ErbaZZ, Rugsurely, doggo, jokopoppo, mimic_f, and rxnnxchxi)
- TPSec (PetarTolev and Pataroff)
- forgebyola
- hihen
- atoko
- Jorgect
- den-sosnowsky
- dimulski
- Zac
- cu5t0mpeo
- Kaysoft
- jolah1
- Autosaida
- alexfilippov314
- n1punp
- Bigsam
- josephdara
- eta
- shaflow2
- btk
- 0xJoyBoy03
- falconhoof
- JanuaryPersimmon2024
- ak1
- recursiveEth
- Cryptor
- 0xbeWater0given
- cheatc0d3
- 0xrex
- bareli
- 0xmystery
- Daniel526
- wangxx2026
- shikhar229169
- Centaur (mazi_xyz, Mylifechangefast_eth, Aristos, and TheWeb3Mechanic)
- Audinarey
- Evo
- 0xDemon
- mrMorningstar
- Giorgio
- Aymen0909
- ironsidesec
- 0xbepresent
- JC
- peanuts
- d_tony7470
- 0xblackskull
- dimah7
- rekxor
- codeslide
- FastChecker
- Stormreckson
- 0x486776
- igbinosuneric
- tofunmi
- slvDev
- Brenzee
- twcctop
- Sparrow
- djanerch
- mrjorystewartbaxter
- 0xSecuri
- DemoreX
- xg
- Stefanov
- PENGUN
- Dup1337 (sorrynotsorry and deliriusz)
- DanielArmstrong
- valy001
- honey-k12
- pavankv
- joicygiore
- John_Femi
- petro_1912
- Topmark
- robertodf99
- 0xmuxyz
- aariiif
- fandonov
- Pelz
- bctester
- Hueber
- LinKenji
- Draiakoo
- codeislight
- seeques
- biakia
- Vancelot
- 0rpse
- novamanbg
- steadyman
- itsabinashb
- Sancybars
- tchkvsky
- HChang26
- lightoasis
- cholakov
- poslednaya
- Ocean_Sky
- havewemeetbefore
- ZdravkoHr
- ChaseTheLight
- web3km
- MrCrowNFT
- youleeyan
- unix515
- nnez
- kk
- sh1v
- eldenelder
- korok
- Nikki
- AvantGard
- gavfu
- 0xch
- Velislav4o
- 4rdiii
- 0xBugSlayer
- 0xMosh
- lanrebayode77
- heedfxn
- BiasedMerc
- Ali-_-Y
This audit was judged by gzeon.
Final report assembled by liveactionllama.
The C4 analysis yielded an aggregated total of 86 unique vulnerabilities. Of these vulnerabilities, 23 received a risk rating in the category of HIGH severity and 63 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 23 reports detailing issues with a risk rating of LOW severity or non-critical.
All of the issues presented here are linked back to their original finding.
The code under review can be found within the C4 NOYA repository, and is composed of 41 smart contracts written in the Solidity programming language and includes 3,999 lines of Solidity code.
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
Note: full discussions can be accessed by viewing each linked submission.
Submitted by rbserver, also found by SpicyMeatball
There are tokens that have token/ETH but no token/USD Chainlink oracles currently in which these tokens have the in scope token behaviors described in https://code4rena.com/audits/2024-04-noya and should be supported by this protocol. To support these tokens, the ETH/USD Chainlink oracle can be used as a part of the route set by the following NoyaValueOracle.updatePriceRoute function for converting the token amount to ETH first and then converting the converted ETH amount to USD.
function updatePriceRoute(address asset, address base, address[] calldata s) external onlyMaintainer {
priceRoutes[asset][base] = s;
emit UpdatedPriceRoute(asset, base, s);
}When converting the value of such token in ETH to USD, the following ChainlinkOracleConnector.getValue function would return getValueFromChainlinkFeed(AggregatorV3Interface(primarySource), amount, getTokenDecimals(decimalsSource), isPrimaryInverse); because amountIn is in ETH's decimals that is 18 and uintprice and sourceTokenUnit are in USD's decimals that is 8 in the getValueFromChainlinkFeed function below, the value returned by the ChainlinkOracleConnector.getValue function is in ETH's decimals. Hence, when converting such token amount to ETH first and then to USD, the value of the corresponding token amount is in ETH's decimals instead of USD's decimals. In comparison, if the token/USD oracle could exist and be used for such token, the value of such token returned by ChainlinkOracleConnector.getValue function would be in USD's decimals instead of ETH's decimals. Thus, the value of such token is much higher when using the token/ETH and ETH/USD oracles indirectly comparing to using the token/USD oracle directly given if such token/USD oracle can become existent in the future. If such token/USD oracle does become existent and be used in the ChainlinkOracleConnector contract in the future, the value of such token amount calculated previously using the token/ETH and ETH/USD oracles indirectly would be incorrectly much higher than the value of the same token amount newly calculated using the token/USD oracle directly.
function getValue(address asset, address baseToken, uint256 amount) public view returns (uint256) {
if (asset == baseToken) {
return amount;
}
(address primarySource, bool isPrimaryInverse) = getSourceOfAsset(asset, baseToken);
if (primarySource == address(0)) {
revert NoyaChainlinkOracle_PRICE_ORACLE_UNAVAILABLE(asset, baseToken, primarySource);
}
address decimalsSource = isPrimaryInverse ? baseToken : asset;
decimalsSource = decimalsSource == ETH || decimalsSource == USD ? primarySource : decimalsSource;
return getValueFromChainlinkFeed(
AggregatorV3Interface(primarySource), amount, getTokenDecimals(decimalsSource), isPrimaryInverse
);
} function getTokenDecimals(address token) public view returns (uint256) {
uint256 decimals = IERC20Metadata(token).decimals();
return 10 ** decimals;
} function getValueFromChainlinkFeed(
AggregatorV3Interface source,
uint256 amountIn,
uint256 sourceTokenUnit,
bool isInverse
) public view returns (uint256) {
int256 price;
uint256 updatedAt;
(, price,, updatedAt,) = source.latestRoundData();
uint256 uintprice = uint256(price);
if (block.timestamp - updatedAt > chainlinkPriceAgeThreshold) {
revert NoyaChainlinkOracle_DATA_OUT_OF_DATE();
}
if (price <= 0) {
revert NoyaChainlinkOracle_PRICE_ORACLE_UNAVAILABLE(address(source), address(0), address(0));
}
if (isInverse) {
return (amountIn * sourceTokenUnit) / uintprice;
}
return (amountIn * uintprice) / (sourceTokenUnit);
}Please add the following test in testFoundry\testOracle.sol. This test will pass to demonstrate the described scenario.
function test_assetTokenValueIsIncorrectWhenETHUSDChainlinkOracleIsNeeded() public {
// Some tokens do not have token/USD oracle so token/ETH and ETH/USD oracles need to be used.
// Following code compares method using token/ETH and ETH/USD oracles indirectly to method using token/USD oracle directly.
address ETH_USD_FEED = 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419;
vm.startPrank(owner);
addTokenToChainlinkOracle(address(USDC), address(840), address(USDC_USD_FEED));
addTokenToChainlinkOracle(address(USDC), address(0), address(USDC_ETH_FEED));
addTokenToChainlinkOracle(address(0), address(840), address(ETH_USD_FEED));
addTokenToNoyaOracle(address(USDC), address(chainlinkOracle));
addTokenToNoyaOracle(address(0), address(chainlinkOracle));
addTokenToNoyaOracle(address(840), address(chainlinkOracle));
uint256 valueDirect = noyaOracle.getValue(address(USDC), address(840), 1e6);
// when using USDC/USD oracle directly, 1e6 wei USDC is equivalent to 99998495 wei USD, which is in USD's decimals that is 8
assertEq(valueDirect, 99998495);
address[] memory assets = new address[](1);
assets[0] = address(0);
noyaOracle.updatePriceRoute(address(USDC), address(840), assets);
uint256 valueIndirect = noyaOracle.getValue(address(USDC), address(840), 1e6);
// when using USDC/ETH and ETH/USD oracles indirectly, 1e6 wei USDC is equivalent to 998152930103816659 wei USD, which is in ETH's decimals that is 18
assertEq(valueIndirect, 998152930103816659);
// value of 1e6 wei USDC is incorrectly much higher when using USDC/ETH and ETH/USD oracles indirectly comparing to using USDC/USD oracle directly
assertEq(valueIndirect / valueDirect, 9981679525);
vm.stopPrank();
}getValueFromChainlinkFeed(AggregatorV3Interface(primarySource), amount, getTokenDecimals(decimalsSource), isPrimaryInverse) returned by the ChainlinkOracleConnector.getValue function can be further divided by 10 ** 10 when asset is ETH, baseToken is USD, and isPrimaryInverse is false. This makes such return value be in USD's decimals that is 8.
gzeon (judge) increased severity to High and commented:
I think I see the problem, when decimalsSource is ETH/USD it treats the decimal as source.decimal() because they are not real ERC20. It is usually fine because ETH oracles are 18 decimals and USD oracles are 8 decimals which is same as the expected value. However, when ETH/USD oracle is used the logic would always consider it as 8 decimal (because ETH/USD oracle as 8 decimal) but in fact it should be treated as 18 decimal if we want the ETH decimal, hence the result is off by 10 decimals.
[H-02] Base tokens like USDT, USDC having different decimals on different chains can have their TVL updated incorrectly
Submitted by MrPotatoMagic, also found by KupiaSec and Weed0607
Tokens like USDT, USDC on the BSC chain have 18 decimals while the "Base" blockchain which is being used as the root/base chain uses 6 decimals (see here).
The issue is that when the TVL is updated on the base chain by sending a cross-chain message call from OmnichainManagerNormalChain.sol contract to OmnichainManagerBaseChain.sol contract, the TVL is recorded in the holding position with 18 decimals instead of scaling it down to 6 decimals.
Due to this, whenever the accounting manager retrieves the TVL() through function totalAssets() (overriden from ERC4626), the TVLHelper contract would loop through all holding positions and sum them up in order to be used in vault operations like minting shares etc. This would be problematic since the tvl is 1e12 decimals more than the actual of 1e6. This breaks the accounting in the accounting manager contract.
Here's the whole process:
- Manager calls function updateTVLInfo() on the OmnichainManagerNormalChain.sol contract on the BSC chain.
- On Line 29, it calls the getTVL() function that loops through all the holding positions for the base token and vaultId.
- The value is returned (in 18 decimals since we're on BSC) and stored in
tvlon Line 29.
Line 30 calls the updateTVL() function on the lzhelper contract, which sends the cross-chain message with the tvl update.
File: OmnichainManagerNormalChain.sol
19: function getTVL() public view returns (uint256) {
20: (, address baseToken) = registry.getVaultAddresses(vaultId);
21: return TVLHelper.getTVL(vaultId, registry, baseToken);
22: }
23: /**
24: * Triggers an update of the vault's TVL information, sending the latest data to the base chain via the LZHelperSender contract.
25: * This function is restricted to be called by managers only, ensuring that TVL updates are controlled and authorized.
26: */
27:
28: function updateTVLInfo() external onlyManager {
29: uint256 tvl = getTVL();
30: LZHelperSender(lzHelper).updateTVL(vaultId, tvl, block.timestamp);
31: }- The call arrives on the base chain by LZ calling the _lzReceive() function, which calls the updateTVL() function on the OmnichainManagerBaseChain.sol contract.
- On Line 39, the
tvlvariable is stored as is with the 18 decimals incorrectly.
File: OmnichainManagerBaseChain.sol
32: function updateTVL(uint256 chainId, uint256 tvl, uint256 updateTime) external nonReentrant {
33: if (msg.sender != lzHelper) revert IConnector_InvalidSender();
34:
35: registry.updateHoldingPostionWithTime(
36: vaultId,
37: registry.calculatePositionId(address(this), OMNICHAIN_POSITION_ID, abi.encode(chainId)),
38: "",
39: abi.encode(tvl),
40: tvl <= DUST_LEVEL,
41: updateTime
42: );
43: }- Now when the totalAssets() function in the accounting manager calls the TVL() function, it loops through the holding positions in the TVLHelper contract and sums up the TVL incorrectly.
function getTVL(uint256 vaultId, PositionRegistry registry, address baseToken) public view returns (uint256) {
uint256 totalTVL;
uint256 totalDebt;
HoldingPI[] memory positions = registry.getHoldingPositions(vaultId);
for (uint256 i = 0; i < positions.length; i++) {
if (positions[i].calculatorConnector == address(0)) {
continue;
}
uint256 tvl = IConnector(positions[i].calculatorConnector).getPositionTVL(positions[i], baseToken);
bool isPositionDebt = registry.isPositionDebt(vaultId, positions[i].positionId);
if (isPositionDebt) {
totalDebt += tvl;
} else {
totalTVL += tvl;
}
}
if (totalTVL < totalDebt) {
return 0;
}
return (totalTVL - totalDebt);
}Through this we observe how incorrect tvl is reported from the normal chain to the base chain, which hugely affects the value retrieved by function totalAssets() for ERC4626 vault operations.
Keep track of the decimals of tokens on other chains in a mapping. If they are more or less, consider scaling them once the tvl update is received on the base chain.
HadiEsna (NOYA) confirmed and commented:
Fix in commit 58537ef76afa048745f85a31cb7a0aa3eb6b300b.
Submitted by TPSec (View additional reports submitted by other wardens)
The NoyaValueOracle.getValue is used to convert the price of an asset to baseToken, using token routing when necessary.
If there is no oracle for two tokens, a route needs to be set up between them. This route can then be used to get a value using NoyaValueOracle.getValue.
However, there's a mistake in the NoyaValueOracle.getValue function that makes it give the wrong price.
The NoyaValueOracle.getValue will return incorrect prices for tokens when the configured priceRoutes[asset][baseToken].length is more than 1.
To set a price path for a token pair, the maintainer should use NoyaValueOracle.updatePriceRoute.
For instance, for the SOL to UNI conversion. 1 SOL is roughly equal to 21 UNI, but there's no direct oracle. So, the maintainer sets a route SOL → BNB → ETH → UNI.
When _getValue tries to convert 100 SOL into UNI, it should first convert SOL → BNB, then BNB → ETH, and lastly ETH → UNI. But, what _getValue really does is to convert SOL → BNB, SOL → ETH, and SOL → UNI.
You can see this error in the _getValue function, where asset is used each time instead of quotingToken.
function _getValue(address asset, address baseToken, uint256 amount, address[] memory sources)
internal
view
returns (uint256 value)
{
uint256 initialValue = amount;
address quotingToken = asset;
for (uint256 i = 0; i < sources.length; i++) {
> initialValue = _getValue(asset, sources[i], initialValue);
quotingToken = sources[i];
}
return _getValue(quotingToken, baseToken, initialValue);
}The first problem comes up if the SOL → BNB, SOL → ETH, and SOL → UNI oracles are not set up. This will cause a revert of _getValue.
function _getValue(address quotingToken, address baseToken, uint256 amount) internal view returns (uint256) {
INoyaValueOracle oracle = priceSource[quotingToken][baseToken];
if (address(oracle) == address(0)) {
oracle = priceSource[baseToken][quotingToken];
}
if (address(oracle) == address(0)) {
oracle = defaultPriceSource[baseToken];
}
if (address(oracle) == address(0)) {
oracle = defaultPriceSource[quotingToken];
}
if (address(oracle) == address(0)) {
> revert NoyaOracle_PriceOracleUnavailable(quotingToken, baseToken);
}
return oracle.getValue(quotingToken, baseToken, amount);
}Even if we configure these oracles, the calculated value will still be incorrect.
Consider this example - the _getValue is called with the following parameters:
- asset = SOL
- baseToken = UNI
- amount = 100
- sources = [BNB, ETH]
This will convert 100 SOL to UNI, using BNB and ETH as the route.
| Iteration | initialValue | quotingToken | asset | sources[i] | initialValue’ | quotingToken’ |
|---|---|---|---|---|---|---|
| 1 | 100 | SOL | SOL | BNB | 100SOL ≈ 25BNB | BNB |
| 2 | 25 | BNB | SOL | ETH | 25BNB ≈ 1.25ETH | ETH |
The result is calculated using quotingToken = ETH, baseToken = UNI, and the initialValue = 1.25, which returns 526.
However, the expected result should be 2094.
Replace asset with quotingToken:
function _getValue(address asset, address baseToken, uint256 amount, address[] memory sources)
internal
view
returns (uint256 value)
{
uint256 initialValue = amount;
address quotingToken = asset;
for (uint256 i = 0; i < sources.length; i++) {
- initialValue = _getValue(asset, sources[i], initialValue);
+ initialValue = _getValue(quotingToken, sources[i], initialValue);
quotingToken = sources[i];
}
return _getValue(quotingToken, baseToken, initialValue);
}HadiEsna (NOYA) confirmed and commented:
Fix in commit 66d4104e83cc1b68ab117c78c4a92eafac86341c.
Submitted by TPSec (View additional reports submitted by other wardens)
https://github.com/d-xo/weird-erc20#tokens-with-blocklists
Some tokens (e.g.
USDC,USDT) have a contract level admin controlled address blocklist. If an address is blocked, then transfers to and from that address are forbidden.
Malicious or compromised token owners can trap funds in a contract by adding the contract address to the blocklist. This could potentially be the result of regulatory action against the contract itself, against a single user of the contract (e.g. a Uniswap LP), or could also be a part of an extortion attempt against users of the blocked contract.
If a user whose address has been blocklisted is added to a withdrawQueue inside the AccountingManager contract, all other users that are in that same queue will not be able to withdraw, as the executeWithdraw function will revert when it tries to do baseToken.safeTransfer call on the blocklisted address.
A user who has been blocklisted can be added to a withdrawQueue to DoS the call of the executeWithdraw function, effectively preventing all other user that are in that same queue from withdrawing, locking their assets inside the protocol.
When baseToken has blocklisting functionality, and any user in the withdrawal queue is in the blocklist, it prevents all other users from making withdrawals.
Place the following test in testFoundry/BlacklistableTokenPOC.t.sol and run it with the command forge test --mt testBlacklistableERC20
// SPDX-License-Identifier: Apache-2.0
pragma solidity 0.8.20;
import "@openzeppelin/contracts-5.0/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts-5.0/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts-5.0/token/ERC20/ERC20.sol";
import "./utils/testStarter.sol";
import "./utils/resources/OptimismAddresses.sol";
import { AaveConnector, BaseConnectorCP } from "contracts/connectors/AaveConnector.sol";
contract BlacklistERC20 is ERC20 {
mapping(address => bool) private blacklisted;
error AccountBlacklisted();
constructor() ERC20("Blacklist ERC20", "BLT") {
_mint(msg.sender, 100e18);
}
function addToBlacklist(address account) public {
blacklisted[account] = true;
}
function isBlacklisted(address account) public view returns (bool) {
return blacklisted[account];
}
function _update(
address from,
address to,
uint256 value
) internal override {
if (isBlacklisted(from) || isBlacklisted(to)) revert AccountBlacklisted();
super._update(from, to, value);
}
}
contract BlacklistableTokenPOC is testStarter, OptimismAddresses {
using SafeERC20 for IERC20;
AaveConnector connector;
address charlie = makeAddr("charlie");
address public constant USD = address(840);
BlacklistERC20 token;
function setUp() public {
vm.label(alice, "alice");
vm.label(bob, "bob");
vm.label(charlie, "charlie");
vm.startPrank(owner);
token = new BlacklistERC20();
deployEverythingNormal(address(token));
connector = new AaveConnector(aavePool, USD, BaseConnectorCP(registry, 0, swapHandler, noyaOracle));
addConnectorToRegistry(vaultId, address(connector));
addTrustedTokens(vaultId, address(accountingManager), address(token));
registry.addTrustedPosition(vaultId, connector.AAVE_POSITION_ID(), address(connector), true, false, "", "");
registry.addTrustedPosition(vaultId, 0, address(accountingManager), false, false, abi.encode(address(token)), "");
vm.stopPrank();
}
function testBlacklistableERC20() public {
uint depositAmount = 10e18;
// give tokens to the users
vm.startPrank(owner);
token.transfer(alice, depositAmount);
token.transfer(bob, depositAmount);
token.transfer(charlie, depositAmount);
vm.stopPrank();
// user's deposits
vm.startPrank(alice);
SafeERC20.forceApprove(IERC20(token), address(accountingManager), depositAmount);
accountingManager.deposit(address(alice), depositAmount, address(0));
vm.stopPrank();
vm.startPrank(bob);
SafeERC20.forceApprove(IERC20(token), address(accountingManager), depositAmount);
accountingManager.deposit(address(bob), depositAmount, address(0));
vm.stopPrank();
vm.startPrank(charlie);
SafeERC20.forceApprove(IERC20(token), address(accountingManager), depositAmount);
accountingManager.deposit(address(charlie), depositAmount, address(0));
vm.stopPrank();
// manager execute deposits
vm.startPrank(owner);
accountingManager.calculateDepositShares(10);
skip(accountingManager.depositWaitingTime());
accountingManager.executeDeposit(10, address(connector), "");
vm.stopPrank();
// withdraw requests
vm.startPrank(alice);
accountingManager.withdraw(accountingManager.maxRedeem(alice), address(alice));
vm.stopPrank();
vm.startPrank(bob);
accountingManager.withdraw(accountingManager.maxRedeem(bob), address(bob));
vm.stopPrank();
vm.startPrank(charlie);
accountingManager.withdraw(accountingManager.maxRedeem(charlie), address(charlie));
vm.stopPrank();
// execute withdraw
vm.startPrank(owner);
accountingManager.calculateWithdrawShares(10);
accountingManager.startCurrentWithdrawGroup();
uint neededAssetsForWithdraw = accountingManager.neededAssetsForWithdraw();
RetrieveData[] memory retrieveData = new RetrieveData[](1);
retrieveData[0] = RetrieveData(
neededAssetsForWithdraw,
address(connector),
abi.encode(neededAssetsForWithdraw, ""));
accountingManager.retrieveTokensForWithdraw(retrieveData);
skip(accountingManager.withdrawWaitingTime());
accountingManager.fulfillCurrentWithdrawGroup();
// @audit When one of the users is blacklisted from the baseToken, no one else can withdraw either
token.addToBlacklist(alice);
vm.expectRevert();
accountingManager.executeWithdraw(10);
}
}Validate that the receiver is not blacklisted before making the ERC20::safeTransfer call, whenever the baseToken has a contract level admin controlled address blocklist (e.g. USDC, USDT).
function executeWithdraw(uint256 maxIterations) public onlyManager nonReentrant whenNotPaused {
...
while (
currentWithdrawGroup.lastId > firstTemp
&& withdrawQueue.queue[firstTemp].calculationTime + withdrawWaitingTime <= block.timestamp // @audit-info use withdrawRequest time
&& i < maxIterations
) {
i += 1;
WithdrawRequest memory data = withdrawQueue.queue[firstTemp];
uint256 shares = data.shares;
// calculate the base token amount that the user will receive based on the total available amount
uint256 baseTokenAmount =
data.amount * currentWithdrawGroup.totalABAmount / currentWithdrawGroup.totalCBAmountFullfilled;
withdrawRequestsByAddress[data.owner] -= shares;
_burn(data.owner, shares);
processedBaseTokenAmount += data.amount;
{
uint256 feeAmount = baseTokenAmount * withdrawFee / FEE_PRECISION;
withdrawFeeAmount += feeAmount;
baseTokenAmount = baseTokenAmount - feeAmount;
}
+ if (baseToken.isBlacklisted(data.receiver)) {
+ continue;
+ }
baseToken.safeTransfer(data.receiver, baseTokenAmount);
emit ExecuteWithdraw(
firstTemp, data.owner, data.receiver, shares, data.amount, baseTokenAmount, block.timestamp
);
delete withdrawQueue.queue[firstTemp];
firstTemp += 1;
}
...
}gzeon (judge) increased severity to High and commented:
High risk because the DOS can cause other funds to be stuck. There are multiple ways to trigger the problem:
- Transfer to address(0)
- Transfer of 0 amount
- Blacklisted receiver
I consider these all to share the same root cause of the deposit and withdrawal queue being blocking.
HadiEsna (NOYA) confirmed and commented:
Fix in commit c9e03316c7f30f5aebe88627fc296763f6105c31.
Submitted by alexxander
PendleConnector contracts can lose out on funds when minting LP tokens.
The PendleMarketV3.mint() function accepts as parameters uint256 netSyDesire and uint256 netPtDesired , which serve as upper bounds, where the Market will try and mint as many LP tokens as possible such that neither of the upper bound amounts are exceeded. The issue is that in PendleConnector.depositIntoMarket, the developers incorrectly assume a call to PendleMarketV3.skim() will return back any unused tokens that were sent. The skim() function sends the extra tokens to the Pendle treasury rather than the calling contract: skim().
* @dev skim allows us to get the surplus tokens
*/
function depositIntoMarket(IPMarket market, uint256 SYamount, uint256 PTamount) external onlyManager nonReentrant {
(IPStandardizedYield _SY, IPPrincipalToken _PT,) = IPMarket(market).readTokens();
IERC20(address(_SY)).safeTransfer(address(market), SYamount);
IERC20(address(_PT)).safeTransfer(address(market), PTamount);
market.mint(address(this), SYamount, PTamount);
market.skim();
emit DepositIntoMarket(address(market), SYamount, PTamount);
}Remove the call to market.skim() and integrate with MarketMathCore.addLiquidity() to pre-calculate the used SY and PT amounts which then can be supplied to market.mint(). This will ensure no tokens are lost.
HadiEsna (NOYA) confirmed and commented:
True, needs to be fixed.
I don't think this one is a High severity because although the use-case of skim() function is incorrectly assumed, by putting correct inputs in the SY and PT amount, the contract is not going to lose any funds.
I think this is High risk due to MEV mentioned in issue #473, even if the UI handles SY and PT amount correctly, funds can still be stolen.
Submitted by KupiaSec, also found by Weed0607, pkqs90 (1, 2), said, Inspex, BenRai (1, 2), Zac, and 0xAlix2
The AerodromeConnector::stake function allows to stake liquidity tokens to the gauge pool. However, the AerodromeConnector::_getPositionTVL function, which is responsible for calculating the Total Value Locked (TVL), does not take into account the liquidity tokens deposited in the gauge pool. This oversight will result in an incorrect calculation of the TVL, which could negatively impact the overall functionality of the protocol that utilizes the TVL metric.
The AerodromeConnector::stake function allows to deposit and stake liquidity tokens in the gauge pool.
AerodromeConnector.sol
100: function stake(address pool, uint256 liquidity) public onlyManager nonReentrant {
101: address gauge = voter.gauges(pool);
102: IERC20(pool).forceApprove(address(gauge), liquidity);
103: IGauge(gauge).deposit(liquidity, address(this)); // @audit-info this sends tokens to the gauge
104: }The implementation of the Gauge::deposit is as follows:
function deposit(uint256 _amount, address _recipient) external {
_depositFor(_amount, _recipient);
}
function _depositFor(uint256 _amount, address _recipient) internal nonReentrant {
if (_amount == 0) revert ZeroAmount();
if (!IVoter(voter).isAlive(address(this))) revert NotAlive();
address sender = _msgSender();
_updateRewards(_recipient);
IERC20(stakingToken).safeTransferFrom(sender, address(this), _amount); // @audit-info liquidity tokens are sent to the gauge.
totalSupply += _amount;
balanceOf[_recipient] += _amount;
emit Deposit(sender, _recipient, _amount);
}As evident from the preceding code snippet, when the Gauge::deposit function is invoked, staked liquidity tokens are sent to the gauge pool. However, the implementation of the AerodromeConnector::_getPositionTVL function only accounts for the liquidity tokens held in the Aerodrome pool, and does not consider the tokens that have been staked in the gauge pool. This oversight in the AerodromeConnector::_getPositionTVL function will result in an incomplete calculation of the Total Value Locked (TVL).
AerodromeConnector.sol
125: function _getPositionTVL(HoldingPI memory p, address base) public view override returns (uint256) {
126: PositionBP memory pBP = registry.getPositionBP(vaultId, p.positionId);
127: (address pool) = abi.decode(pBP.data, (address));
128: uint256 balance = IERC20(pool).balanceOf(address(this)); // @audit-issue only accounts the liquidity token balance at the aerodrome pool, does not include the gauge pool
129: uint256 totalSupply = IERC20(pool).totalSupply();
130: (uint256 reserve0, uint256 reserve1,) = IPool(pool).getReserves();
131: uint256 amount0 = balance * reserve0 / totalSupply;
132: uint256 amount1 = balance * reserve1 / totalSupply;
133: return _getValue(IPool(pool).token0(), base, amount0) + _getValue(IPool(pool).token1(), base, amount1);
134: }As a result, the Total Value Locked (TVL) value calculated by the AerodromeConnector::_getPositionTVL function will be less than the actual value, as it does not account for the liquidity tokens staked in the gauge pool. This underestimation of the TVL could negatively impact the overall functionality of the protocol that relies on the TVL metric.
It is recommended to fix the _getPositionTVL function to take account liquidity tokens staked in the gauge pool as follows:
function _getPositionTVL(HoldingPI memory p, address base) public view override returns (uint256) {
PositionBP memory pBP = registry.getPositionBP(vaultId, p.positionId);
(address pool) = abi.decode(pBP.data, (address));
+ address gauge = voter.gauges(pool);
- uint256 balance = IERC20(pool).balanceOf(address(this));
+ uint256 balance = IERC20(pool).balanceOf(address(this)) + IERC20(gauge).balanceOf(address(this));
uint256 totalSupply = IERC20(pool).totalSupply();
(uint256 reserve0, uint256 reserve1,) = IPool(pool).getReserves();
uint256 amount0 = balance * reserve0 / totalSupply;
uint256 amount1 = balance * reserve1 / totalSupply;
return _getValue(IPool(pool).token0(), base, amount0) + _getValue(IPool(pool).token1(), base, amount1);
}HadiEsna (NOYA) confirmed and commented:
True, needs to be fixed.
But I think Medium severity not High, because it's not going to cause loss of funds in any way.
Finalizing as High Risk as TVL affects share calculation.
Submitted by alexxander, also found by ArsenLupin, WinSec, and 0xAlix2
Burning a Pendle LP will donate the PT tokens to the market instead of withdrawing them to the PendleConnector which leads to a loss of funds for the Connector.
The Pendle Market burn() function accepts as the first address parameter the receiver of the SY tokens and a second address parameter that is the receiver of the PT tokens. The issue is that in PendleConnecotr.burnLP(), the function market.burn() selects the market as the receiver of the PT tokens, where it should be the PendleConnector i.e., address(this):
function burnLP(IPMarket market, uint256 amount) external onlyManager nonReentrant {
IERC20(address(market)).safeTransfer(address(market), amount);
// @audit -> address(market) should be address(this)
market.burn(address(this), address(market), amount);
market.skim();
emit BurnLP(address(market), amount);
}// PendleMarket.burn()
function burn(
address receiverSy,
address receiverPt,
uint256 netLpToBurn
) external nonReentrant returns (uint256 netSyOut, uint256 netPtOut) { function burnLP(IPMarket market, uint256 amount) external onlyManager nonReentrant {
IERC20(address(market)).safeTransfer(address(market), amount);
- market.burn(address(this), address(market), amount);
+ market.burn(address(this), address(this), amount);
market.skim();
emit BurnLP(address(market), amount);
}[H-08] A Vault can steal all funds from another Vault through the Registry's flash loan contract due to insufficient access control in Connector.sendTokensToTrustedAddress()
Submitted by alexxander (View multiple reports submitted by additional wardens)
The keeper of a Vault can steal all funds from another Vault.
Noya Vaults are registered with the Registry contract where Registry.addVault() is used to add a particular Vault and specify the address of its AccountingManager, maintainer, keeper, etc. The Registry contract also has the state variable address flashLoan which is the address of a BalancerFlashLoan contract that is used by the Vaults to leverage flash loans. The issue is that in any Connector, the BaseConnector.sendTokensToTrustedAddress() function allows the BalancerFlashLoan contract to perform token transfers back to the BalancerFlashLoan contract - code snippet. This allows for the following exploit:
- Assume 2 Vaults with Ids: ID-1 and ID-2
- Assume ID-1 has a Connector C-1
- Assume ID-2 has a Connector C-2
- Assume C-2 holds X amount of token Y
- The keeper of ID-1 calls
BalancerFlashLoan.makeFlashloan()code snippet - the
tokensarray holds the address of some token - the
amountsarray holds some amount of the token intokens bytes userDatais packed asuint256 vaultId = ID-1address receiver = C-1address[] destinantionConnector = [C-2, Y]bytes[] callingData = [sendTokensToTrustedAddress(Y, X, address(0), ""), transferTo(C1, X)]uint256[] gas = [enoughGas, enoughGas]
BalancerFlashLoan.receiveFlashLoan()is called by Balancer- The
receiver(C-1) get's the flash loan tokens destinationConnector[0].call{...}(callingData[0]), which is theBaseConnector.sendTokensToTrustedAddress()call toC-2. The ConnectorC-2allows the execution sincemsg.senderisregistry.flashLoan().C-2sends X amount of tokens toBalancerFlashLoan.destinationConnector[1].call{...}(callingData[1]), which is theY.transferTo()call that will transfer the X amount of tokens fromBalancerFlashLoantoC-1.C-1returns the borrowed tokens and keeps the X amount of token Y
The aim of the POC is to showcase how one Vault can steal the funds from another Vault's BaseConnector. The test consists of 2 Vaults (ID-5 and ID-10). In the beginning of the test
ID-5 receives and executes deposits that go into its BaseConnector. The keeper of ID-10 then leverages the BalancerFlashLoan contract to call BaseConnector.sendTokensToTrustedAddress() and after that to retrieve the funds into an arbitrary address.
- Add the contents of the following gist in a solidity file under
/testsFoundry- gist with code - Execute with
forge test --match-test testVaultIssue -vv --fork-url <mainnet rpc url>
Function BaseConnector.sendTokensToTrustedAddress() should be able to check that the caller of BalancerFlashLoan.makeFlashLoan() is authorized to perform calls into the Connector.
HadiEsna (NOYA) confirmed and commented:
Fixed in e1d9ac635b6cada731bdfc16d2a1b7b640360311 and 986d24df537bed29ebec7fc51ae2e6f442058cea.
Submitted by grearlake
From prisma docs:
While in Recovery Mode, if your vault’s Individual Collateral Ratio (ICR) falls below the GTCR, your vault can be liquidated (even if your vault's collateral ratio is above 110%). To prevent this from happening, in both Normal and Recovery Mode, a user should maintain their collateral ratio over 150%.
During Recovery Mode, the liquidation loss is capped at 110% of a vault's collateral. Any residual amount, i.e. the collateral above 110% (and below the Global Total Collateral Ratio or GTCR), can be recouped by the borrower who faced liquidation by claiming the surplus collateral.
This implies that a borrower will encounter the same liquidation "penalty" (20%) in Recovery Mode as they would in Normal Mode if their vault undergoes liquidation.
Function claimCollateral() is used to claim surplusBalances link:
function claimCollateral(address _receiver) external {
uint256 claimableColl = surplusBalances[msg.sender];
require(claimableColl > 0, "No collateral available to claim");
surplusBalances[msg.sender] = 0;
collateralToken.safeTransfer(_receiver, claimableColl);
}
But in PrismaConnector contract, it does not have function to call claimCollateral() function, lead to surplusBalances is stucked forever.
surplusBalances are not able to be claimed, funds is stuck.
Create function to call claimCollateral() function in prisma.
Submitted by Aymen0909 (View additional reports submitted by other wardens)
https://github.com/code-423n4/2024-04-noya/blob/main/contracts/accountingManager/AccountingManager.sol#L461-L468
https://github.com/code-423n4/2024-04-noya/blob/main/contracts/accountingManager/AccountingManager.sol#L328-L354
The resetMiddle function in the AccountingManager contract is designed to reset the middle index of either the deposit or withdraw queue. This function is crucial for managing the order and processing of transactions within these queues, particularly in scenarios where recalculations are necessary to ensure fairness and accuracy in the distribution of shares or handling of withdrawals.
The function will work as expected when resetting the deposit queue, but it will not work for the reset of the withdrawal queue and will cause wrong withdrawal amount accounting to occurs.
To understand the issue we must first see how the withdrawal queue middle is calculated. The middle index of the withdrawal queue is updated during the process of calculating the withdrawal shares. This occurs in the calculateWithdrawShares function, which is designed to process queued withdrawal requests and assign the corresponding amount of base tokens to each request based on the current share price.
For each withdrawal request in the queue that has not yet been processed, the function calculates the amount of base tokens equivalent to the shares requested for withdrawal. After processing the maximum allowed iterations or reaching the end of the queue, the function updates the official middle index of the withdrawal queue to the last processed index (middleTemp) and increments the total base token amount needed for the current withdraw group currentWithdrawGroup.totalCBAmount (remember this state as it will be important).
Here is the relevant part of the code that handles the updating of the middle index:
function calculateWithdrawShares(uint256 maxIterations) public onlyManager nonReentrant whenNotPaused {
uint256 middleTemp = withdrawQueue.middle;
uint64 i = 0;
...
while (withdrawQueue.last > middleTemp && i < maxIterations) {
WithdrawRequest storage data = withdrawQueue.queue[middleTemp];
uint256 assets = previewRedeem(data.shares);
data.amount = assets;
data.calculationTime = block.timestamp;
emit CalculateWithdraw(middleTemp, data.owner, data.receiver, data.shares, assets, block.timestamp);
middleTemp += 1;
i += 1;
}
currentWithdrawGroup.totalCBAmount += assetsNeededForWithdraw;
withdrawQueue.middle = middleTemp;
}Now let's see how the resetMiddle function will cause an issue to occur in the withdrawal process:
function resetMiddle(
uint256 newMiddle,
bool depositOrWithdraw
) public onlyManager {
if (depositOrWithdraw) {
emit ResetMiddle(newMiddle, depositQueue.middle, depositOrWithdraw);
if (
newMiddle > depositQueue.middle ||
newMiddle < depositQueue.first
) {
revert NoyaAccounting_INVALID_AMOUNT();
}
depositQueue.middle = newMiddle;
} else {
emit ResetMiddle(
newMiddle,
withdrawQueue.middle,
depositOrWithdraw
);
if (
newMiddle > withdrawQueue.middle ||
newMiddle < withdrawQueue.first ||
currentWithdrawGroup.isStarted
) {
revert NoyaAccounting_INVALID_AMOUNT();
}
withdrawQueue.middle = newMiddle;
//@audit currentWithdrawGroup.totalCBAmount is not reset
}
}As it can be seen the function doesn't reset the value for current withdraw group currentWithdrawGroup.totalCBAmount which will keep its value from the previous calculation which is supposed to be wrong, and thus when later on the manager will call calculateWithdrawShares to calculate the withdrawal shares once again, currentWithdrawGroup.totalCBAmount will get incremented on the previously calculated amount which will result in a bigger base token amount needed for the withdrawal to be processed than what was supposed to be.
Thus the accounting for the current withdrawal group will be wrong. This issue will cause the following problems:
-
First it will force the protocol manager to withdraw more funds than intended from the connectors to reach the amount needed for withdrawal of current group represented by
currentWithdrawGroup.totalCBAmount(which is wrong -bigger- in our case after the reset has occurred), this will result in potential yield that could've been generated and thus a loss for funds for the protocol and its users. -
The worst that can happen is if
currentWithdrawGroup.totalCBAmountafter being incremented twice for the same group (the first time and the second time after the reset call as it wasn't updated) is bigger that the current protocol TVL (which can happen especially in early days of the vault), in that case the protocol is unable to get necessary amount of funds to execute the withdrawal and the withdrawal process and funds will remain blocked until new deposit are made & executed.
The reset function doesn't reset the withdrawal amount requested currentWithdrawGroup.totalCBAmount which will cause problems in the withdrawal process and result a loss of funds for the protocol and all other users.
To address this issue, the reset function must reset the withdrawal amount requested currentWithdrawGroup.totalCBAmount (set back to 0), the function should be modified as follows:
function resetMiddle(
uint256 newMiddle,
bool depositOrWithdraw
) public onlyManager {
if (depositOrWithdraw) {
emit ResetMiddle(newMiddle, depositQueue.middle, depositOrWithdraw);
if (
newMiddle > depositQueue.middle ||
newMiddle < depositQueue.first
) {
revert NoyaAccounting_INVALID_AMOUNT();
}
depositQueue.middle = newMiddle;
} else {
emit ResetMiddle(
newMiddle,
withdrawQueue.middle,
depositOrWithdraw
);
if (
newMiddle > withdrawQueue.middle ||
newMiddle < withdrawQueue.first ||
currentWithdrawGroup.isStarted
) {
revert NoyaAccounting_INVALID_AMOUNT();
}
withdrawQueue.middle = newMiddle;
++ currentWithdrawGroup.totalCBAmount = 0;
}
}gzeon (judge) increased severity to High
HadiEsna (NOYA) confirmed and commented:
Fixed in commit 962d297e937e82d95e20739078a61b00eea47643.
If we are resetting the withdrawal queue, we have to reset all withdrawals that are calculated so far. So we can safely set thetotalCBAmountto 0.
Submitted by pkqs90 (View additional reports submitted by other wardens)
SNXConnector allows depositing collateral and minting SUSD.
There are two issues for TVL calculation code:
- It does not account for the debt occurred for minting SUSD
totalDepositedalready includestotalAssigned
Note that the minted SUSD tokens are already calculated into TVL by calling _updateTokenInRegistry(usdToken); while minting SUSD, so the debt part must be calculated as well.
function mintOrBurnSUSD(
uint256 _amount,
uint128 _accountId,
uint128 poolId,
address collateralType,
bool mintOrBurn
) public onlyManager {
// Mint or burn
address usdToken = SNXCoreProxy.getUsdToken();
if (mintOrBurn) {
SNXCoreProxy.mintUsd(_accountId, poolId, collateralType, _amount);
} else {
_approveOperations(usdToken, address(SNXCoreProxy), _amount);
SNXCoreProxy.burnUsd(_accountId, poolId, collateralType, _amount);
}
_updateTokenInRegistry(collateralType);
> _updateTokenInRegistry(usdToken);
}
function _getPositionTVL(HoldingPI memory p, address base) public view override returns (uint256 tvl) {
(uint128 accountId, address collateralType) = abi.decode(p.data, (uint128, address));
(uint256 totalDeposited, uint256 totalAssigned, uint256 totalLocked) =
SNXCoreProxy.getAccountCollateral(accountId, collateralType);
> tvl = _getValue(collateralType, base, totalDeposited + totalAssigned);
}We can find the implementation of getAccountCollateral here: https://github.com/Synthetixio/synthetix-v3/blob/main/protocol/synthetix/contracts/storage/Account.sol#L101-L116
function getCollateralTotals(
Data storage self,
address collateralType
)
internal
view
returns (uint256 totalDepositedD18, uint256 totalAssignedD18, uint256 totalLockedD18)
{
totalAssignedD18 = getAssignedCollateral(self, collateralType);
> totalDepositedD18 =
> totalAssignedD18 +
> self.collaterals[collateralType].amountAvailableForDelegationD18;
totalLockedD18 = self.collaterals[collateralType].getTotalLocked();
return (totalDepositedD18, totalAssignedD18, totalLockedD18);
}The collateral calculation can just use totalDeposited.
However the debt calculation is a bit tricky. The closest API for fetching debt is using updateAccountDebt https://github.com/Synthetixio/synthetix-v3/blob/main/protocol/synthetix/contracts/storage/Pool.sol#L366-L373, but it is not a view function. Not really sure what the best solution here is.
[H-12] Registry.sol#updateHoldingPosition remove position logic is incorrect: should use ownerConnector instead of calculatorConnector when calculating holdingPositionId
Submitted by pkqs90, also found by Autosaida, josephdara, den-sosnowsky, eta, SeveritySquad, and 0xAlix2
When removing a holdingPosition that is not at the back of holdingPositions array, the holdingPositionId is incorrectly calculated. This will mess up the position index, and ultimately mess up TVL calculation.
When removing a position in updateHoldingPosition, if the index of the position is not the last, it will do a swap between the current index and the last index, and remove the last position in holdingPositions array. However, the issue is when updating vault.isPositionUsed[holdingPositionId] index, the holdingPositionId is not correct.
It should be using vault.holdingPositions[positionIndex].ownerConnector instead of vault.holdingPositions[positionIndex].calculatorConnector.
This would be an issue for most of the token-holding positions because the calculateorConnector is the AccountingManager while the ownerConnector is the connector's own address.
function updateHoldingPosition(
uint256 vaultId,
bytes32 _positionId,
bytes calldata _data,
bytes calldata additionalData,
bool removePosition
) public vaultExists(vaultId) returns (uint256) {
Vault storage vault = vaults[vaultId];
if (!vault.connectors[msg.sender].enabled) revert UnauthorizedAccess();
if (!vault.trustedPositionsBP[_positionId].isEnabled) revert InvalidPosition(_positionId);
bytes32 holdingPositionId = keccak256(abi.encode(msg.sender, _positionId, _data));
uint256 positionIndex = vault.isPositionUsed[holdingPositionId];
if (positionIndex == 0 && removePosition) return type(uint256).max;
if (removePosition) {
if (positionIndex < vault.holdingPositions.length - 1) {
vault.holdingPositions[positionIndex] = vault.holdingPositions[vault.holdingPositions.length - 1];
vault.isPositionUsed[keccak256(
abi.encode(
> vault.holdingPositions[positionIndex].calculatorConnector,
vault.holdingPositions[positionIndex].positionId,
vault.holdingPositions[positionIndex].data
)
)] = positionIndex;
}
vault.holdingPositions.pop();
vault.isPositionUsed[holdingPositionId] = 0;
emit HoldingPositionUpdated(vaultId, _positionId, _data, additionalData, removePosition, positionIndex);
return type(uint256).max;
}
return
updateHoldingPosition(vault, vaultId, _positionId, _data, additionalData, positionIndex, holdingPositionId);
}We will give an example of how this bug will mess up the position index and ultimately lead to incorrect TVL.
Add the following test code in BaseConnector.t.sol. It does the following:
- Create 3 connectors, with the order [1, 2, 3] (with a dummy connector up front), and deal 10e6 USDC to each of them.
- Check that current TVL is 30e6.
- Move all USDC from connector 1 to 3. This should remove connector 1's position. The current position array is [3, 2].
- Move all USDC from connector 3 to 2. This should remove connector 3's position. However, since the
positionIndexis not correctly updated in step 3, the connector 3'spositionIndexis still 3. This inupdateHoldingPosition()will result in popping the position at the back (which is actually position 2). - Check that current TVL is 0: because connector 2 is holding all the USDC but it has been popped out in step 4.
function testRemoveConnectorBug() public {
vm.startPrank(owner);
BaseConnector connector3;
connector3 = new BaseConnector(BaseConnectorCP(registry, 0, swapHandler, noyaOracle));
addConnectorToRegistry(vaultId, address(connector3));
// Step 1: Deal 10e6 USDC tokens to all 3 connectors.
uint256 amount = 10 * 1e6;
_dealWhale(baseToken, address(connector), USDC_Whale, amount);
_dealWhale(baseToken, address(connector2), USDC_Whale, amount);
_dealWhale(baseToken, address(connector3), USDC_Whale, amount);
vm.startPrank(owner);
connector.updateTokenInRegistry(USDC);
connector2.updateTokenInRegistry(USDC);
connector3.updateTokenInRegistry(USDC);
// Make sure the TVL is 30e6.
uint tvl = accountingManager.TVL();
assertEq(tvl, 30e6);
// Step 2: Move all USDC from connector 1 to connector 3. TVL is still 30e6.
address[] memory tokens = new address[](1);
tokens[0] = USDC;
uint256[] memory amounts = new uint256[](1);
amounts[0] = amount;
{
connector.transferPositionToAnotherConnector(tokens, amounts, "", address(connector3));
uint tvl = accountingManager.TVL();
assertEq(tvl, 30e6);
}
// Step 3: Move all USDC from connector 3 to connector 2. TVL is now ZERO.
{
amounts[0] = 2 * amount;
connector3.transferPositionToAnotherConnector(tokens, amounts, "", address(connector2));
uint tvl = accountingManager.TVL();
assertEq(tvl, 0);
}
}Foundry
Use vault.holdingPositions[positionIndex].ownerConnector to calculate holdingPositionId.
HadiEsna (NOYA) confirmed and commented:
Fix in commit aef35f768753277552c65ffecf199ea73d0cd058.
Submitted by SBSecurity, also found by ladboy233, DPS, SeveritySquad, Sentryx, and 0xAlix2
BalancerConnector misuses the weight of pool tokens, leading to decreased TVL of these positions.
If we take a look at the balancer documentation and especially how the tokens are being valued, we can see that they are not using weight to determine the price of the pool.tokens[pool.tokenIndex]:
(tokens, balances, lastChangeBlock) = vault.getPoolTokens(poolId);
prices = fetchPricesFromPriceProvider(tokens); //ex. CoinGecko
poolValueUsd = sum(balances[i]*price[i]);
bptPriceUsd = poolValueUsd/bpt.totalSupply();As we can see they are directly calculate the balance by the price, which is not the case in Noya:
BalancerConnector.sol#L162-L173
function _getPositionTVL(HoldingPI memory p, address base) public view override returns (uint256) {
PositionBP memory PTI = registry.getPositionBP(vaultId, p.positionId);
PoolInfo memory pool = abi.decode(PTI.additionalData, (PoolInfo));
uint256 lpBalance = totalLpBalanceOf(pool);
(, uint256[] memory _tokenBalances,) = IBalancerVault(balancerVault).getPoolTokens(pool.poolId);
uint256 _totalSupply = IERC20(pool.pool).totalSupply();
uint256 _weight = pool.weights[pool.tokenIndex];
uint256 token1bal = valueOracle.getValue(pool.tokens[pool.tokenIndex], base, _tokenBalances[pool.tokenIndex]);
return (((1e18 * token1bal * lpBalance) / _weight) / _totalSupply);
} The reason why weight is not needed here is because the balances returned from getPoolTokens are already split by them. If we look at the DAI-USDC-USDT pool we will see that balances returned are already splitted and there is no need to be divided by their weights. (Note: please see image in warden's original submission)
But since Noya uses weights to divide the balance of the tokens, the result will be amount which is percentage weight, set in the pool.weights[pool.tokenIndex], of the actual balance and TVL will be accounted much lower as it is in reality.
Example with USDT and values from BalancerConnector.t.sol:
return (((1e18 * 423099282190 * 991435857075096399976) / 333333333333333333) / 2596148431740844293012911818494888)≈ 0.484
Do not use weights to evaluate the TVL of the position, instead just refactor the _getPositionTVL's return with proper decimal scaling based on the asset used.
[H-14] BalancerConnector has incorrect implementation of totalSupply, positionTVL and total TVL will be invalid
Submitted by oakcobalt, also found by grearlake, BenRai, pkqs90, DPS, SeveritySquad, Sentryx, and cu5t0mpeo
In BalancerConnector::_getPositionTVL, when calculating connector held LP token ratio, IERC20(pool.pool).totalSupply() is called. This is an incorrect implementation.
In Balancer, totalSupply() is different from totalActualSupply(). Based on Balancer doc:
getActualSupply: This is the most common/current function to call. getActualSupply is used by the most recent versions of Weighted and Stable Pools. It accounts for pre-minted BPT as well as due protocol fees.
totalSupply: In general, totalSupply only makes sense to call for older "legacy" pools. The original Weighted and Stable Pools do not have pre-minted BPT, so they follow the typical convention of using totalSupply to account for issued pool shares.
In short, totalSupply will include pre-minted BPT(balancer pool tokens) and shouldn't be used when calculating user position value. The pool's arithmetic behaves as if it didn't exist, and the BPT total supply is not a useful value.
getAcutalSupply should be used instead.
//contracts/connectors/BalancerConnector.sol
function _getPositionTVL(HoldingPI memory p, address base) public view override returns (uint256) {
PositionBP memory PTI = registry.getPositionBP(vaultId, p.positionId);
PoolInfo memory pool = abi.decode(PTI.additionalData, (PoolInfo));
uint256 lpBalance = totalLpBalanceOf(pool);
(, uint256[] memory _tokenBalances,) = IBalancerVault(balancerVault).getPoolTokens(pool.poolId);
|> uint256 _totalSupply = IERC20(pool.pool).totalSupply();
uint256 _weight = pool.weights[pool.tokenIndex];
uint256 token1bal = valueOracle.getValue(pool.tokens[pool.tokenIndex], base, _tokenBalances[pool.tokenIndex]);
|> return (((1e18 * token1bal * lpBalance) / _weight) / _totalSupply);
}In addition, in BalancerConnecot.t.sol we can see the intention is to use weighted pools with pre-minted BPT(USDC-DAI-USDT Stable Pool).
Based on the deployed pool contract, totalSupply is 2596148430608596515167161432296901 . totalAcutalSupply is 1036029413274776191102780.
Use IERC20(pool.pool).totalActualSupply() with compatible pools.
HadiEsna (NOYA) confirmed and commented:
True, needs to be fixed. But not high severity I think.
Considering as High Risk due to TVL impact share calculation.
Submitted by AlexCzm, also found by ArsenLupin, Sentryx, and 0xAlix2
_getPositionTVL from SiloConnector calculates wrongly the position's TVL thus affecting the share price at which shares are minted / redeemed from the protocol.
Each connector implements a _getPositionTVL used to return the value of funds, in base token, its corresponding protocol holds.
This information is used by AccountingManager to calculate the total assets of the vault and used by 4626 standard to calculate the shares price.
The problem is that SiloConnector miscalculate the value sent to SiloConnector protocol.
function _getPositionTVL(HoldingPI memory p, address base) public view override returns (uint256 tvl) {
PositionBP memory bp = registry.getPositionBP(vaultId, p.positionId);
(address siloToken) = abi.decode(bp.data, (address));
ISilo silo = ISilo(siloRepository.getSilo(siloToken));
(address[] memory assets, IBaseSilo.AssetStorage[] memory assetsS) = silo.getAssetsWithState();
uint256 totalDepositAmount = 0;
uint256 totalBAmount = 0;
for (uint256 i = 0; i < assets.length; i++) {
uint256 depositAmount = IERC20(assetsS[i].collateralToken).balanceOf(address(this));
depositAmount += IERC20(assetsS[i].collateralOnlyToken).balanceOf(address(this));
uint256 borrowAmount = IERC20(assetsS[i].debtToken).balanceOf(address(this));
if (depositAmount == 0 && borrowAmount == 0) {
continue;
}
uint256 price = _getValue(assets[i], base, 1e18);
totalDepositAmount += depositAmount * price / 1e18;
totalBAmount += borrowAmount * price / 1e18;
}
tvl = totalDepositAmount - totalBAmount;
}This function calls silo.getAssetsWithState() to get a list of assets and data associated with them. Then for each asset:
- calculates
depositAmountas sum of 2 different token balances:collateralTokenandcollateralOnlyToken - retrieve the
borrowAmountas the balance of a 3rd token nameddebtToken. - get the price of asset (eg.
assets[i]) in 'base' token - calculates and save the deposited and borrowed amounts
After all assetes have been looped over, the tvl is calculated as the difference between deposits and debt amounts:
tvl - totalDepositAmount - totalBAmount
There are 2 problems here:
collateralTokenandcollateralOnlyTokenare 2 different tokens and protocol is summing them as if they represents amounts of the same token.- protocol is multiplying the price of
assets[i]by the amounts of shares and not the underlying token (which isassets[i]).
All 3 tokens ( collateralToken, collateralOnlyToken and debtToken) are share tokens with the same underlying asset but have different purposes and different exchange rate.
To help us creating a better picture of what each token represent we can look how AssetStorage struct looks like:
/// @dev Storage struct that holds all required data for a single token market
struct AssetStorage {
/// @dev Token that represents a share in totalDeposits of Silo
IShareToken collateralToken;
/// @dev Token that represents a share in collateralOnlyDeposits of Silo
IShareToken collateralOnlyToken;
/// @dev Token that represents a share in totalBorrowAmount of Silo
IShareToken debtToken;
/// @dev COLLATERAL: Amount of asset token that has been deposited to Silo with interest earned by depositors.
/// It also includes token amount that has been borrowed.
uint256 totalDeposits;
/// @dev COLLATERAL ONLY: Amount of asset token that has been deposited to Silo that can be ONLY used
/// as collateral. These deposits do NOT earn interest and CANNOT be borrowed.
uint256 collateralOnlyDeposits;
/// @dev DEBT: Amount of asset token that has been borrowed with accrued interest.
uint256 totalBorrowAmount;
}To make an analogy, it is like summing peanuts amount with fruits amount and then multiplying it by the peanut butter jelly price.
In the for loop, after you got the share tokens balances with balanceOf:
- convert all 3 share tokens balances to underlying token (eg.
underlyingTotalDeposits, underlyingCollateralOnlyDeposits, underlyingBorrowAmount) - sum underlying deposits :
underlyingTotalDeposits = underlyingTotalDeposits + underlyingCollateralOnlyDeposits - get the price of the underlying asset
- calculate the
totalDepositAmountandtotalBAmountusing the calculatedunderlyingTotalDepositsandunderlyingBorrowAmountand price.
[H-16] It is possible to open insolvent position in Silo connector, due to missing check in borrow function
Submitted by KYP, also found by KupiaSec, Weed0607, pkqs90, AlexCzm, ladboy233, Bigsam, BenRai (1, 2), 0xAlix2, and Tigerfrake
NOYA uses Connectors to interact with different protocols - deposit, borrow, repay, withdraw. One of the protocols is Silo through the SiloConnector.
In the code snippet below the borrow and repay functions of the SiloConnector can be seen:
function borrow(address siloToken, address bToken, uint256 amount) external onlyManager nonReentrant {
ISilo silo = ISilo(siloRepository.getSilo(siloToken));
silo.borrow(bToken, amount);
emit Borrow(siloToken, bToken, amount);
}
...
function repay(address siloToken, address rToken, uint256 amount) external onlyManager nonReentrant {
ISilo silo = ISilo(siloRepository.getSilo(siloToken));
_approveOperations(rToken, address(silo), amount);
silo.repay(rToken, amount);
_updateTokenInRegistry(rToken);
if (!SolvencyV2.isSolvent(silo, address(this), minimumHealthFactor)) {
revert IConnector_LowHealthFactor(0);
}
emit Repay(siloToken, rToken, amount);
}As can be seen in the repay() function there is a solvency check that reverts the transaction if the repay amount does not make to position solvent again.
However, this check is missing in the borrow() function which allows for a position (in Silo) to be opened that is insolvent (according to the NOYA solvency check in SolvencyV2) from the start.
Missing Validation
Breaks protocol invariant of minimum health factor and also the position is easier to get liquidated.
Add the following test to SiloConnecctor.t.sol and run with forge test --mt test_borrowAndRepay -vv.
function test_borrowAndRepay() public {
// Get value of 1 WETH in USDC
uint256 wethPrice = chainlinkOracle.getValue(address(WETH), address(840), 1 ether);
console.log("weth price :", wethPrice); // 2903,25841858
// Deal a bit more (15%) USDC than the amount of 2 ETH. Mul by 23 and div by 10 so we deposit 115% USDC per ETH - 23/2 ETH = 1.15
uint256 _amount = wethPrice * 23 / 100 / 10; // Divide by 100 because oracle price is 8 dec and USDC is 6.
console.log("USDC deposited:", _amount);
_dealWhale(baseToken, address(connector), USDC_Whale, _amount);
vm.startPrank(address(owner));
// siloToken, deposit token, amount, open position
connector.deposit(USDC, USDC, _amount, true);
// siloToken, borrow token, amount
connector.borrow(USDC, WETH, 2e18); // borrow 2 WETH in exchange for ~7000 USDC as collateral
// Get silo repository from connector
ISiloRepository siloRepository1 = connector.siloRepository();
// Get USDC silo from silo repository
ISilo silo = ISilo(siloRepository1.getSilo(USDC));
console.log("Is Solvent: %s ", SolvencyV2.isSolvent(silo, address(connector), connector.minimumHealthFactor()));
// siloToken, repay token, amount
connector.repay(USDC, WETH, 1e18);
vm.stopPrank();
}Add solvency check in the borrow function.
function borrow(address siloToken, address bToken, uint256 amount) external onlyManager nonReentrant {
ISilo silo = ISilo(siloRepository.getSilo(siloToken));
silo.borrow(bToken, amount);
+ if (!SolvencyV2.isSolvent(silo, address(this), minimumHealthFactor)) {
+ revert IConnector_LowHealthFactor(0);
+ }
_updateTokenInRegistry(bToken);
emit Borrow(siloToken, bToken, amount);
}Submitted by KYP
The PrismaConnector contract is responsible for minting ULTRA and managing the collateral backing it. When a trove is opened, collateral is put up and ULTRA minted to the PrismaConnector. The contract also allows to manage how much of the stablecoin is minted or repay some of it, to improve the collateral ratio (CR). Since collateral can be liquidated an additional check is made to ensure adjustment operations don't leave the trove insolvent:
uint256 healthFactor = ITroveManager(tm).getNominalICR(address(this));
if (minimumHealthFactor > healthFactor) {
revert IConnector_LowHealthFactor(healthFactor);
}The issue lies in the fact that getNominalICR is used to get the health factor (aka CR) and the resulting value is always less than minimumHealthFactor (1.5e18). Unless an unreasonable amount of collateral is put up. Thus, adjustTrove becomes practically unusable to borrow or repay.
Incorrect CR check in PrismaConnector.
Adjusting trove in the PrismaConnector is broken. Once a trove is opened it can only be closed. In the worst case scenario, not enough ULTRA is initially minted to close the trove, thus getting back the collateral will become impossible.
The following test demonstrates how adjusting the trove fails once it's opened.
Keep in mind that the only part that fails is the wrongly checked CR, actual interaction with Prisma works. Which means that the trove has enough collateral to adjust.
In ITroveManager add the following method:
function getCurrentICR(address _borrower, uint256 _price) external view returns (uint256);In PrismaConnector.t.sol add the following test and run with forge test --mt testTroveHealthFactor -vvv:
function testTroveHealthFactor() public {
uint256 amount = 2e18; // collateral
uint256 ultraToMint = 2000e18;
_dealERC20(WETH, address(connector), amount);
uint256 wethPrice = chainlinkOracle.getValue(address(WETH), address(840), 1 ether);
console.log("wethPrice :", wethPrice); // 3037$ per WETH
vm.startPrank(owner);
connector.approveZap(IStakeNTroveZap(ULTRA_StakeNTroveZap), ULTRA_weeth_troveManager, true);
connector.openTrove(
IStakeNTroveZap(ULTRA_StakeNTroveZap), ULTRA_weeth_troveManager,
125e14, // max fee
amount, // collateral
ultraToMint // ULTRA to mint
);
ITroveManager tm = ITroveManager(ULTRA_weeth_troveManager);
(uint256 coll1, uint256 debt1) = tm.getTroveCollAndDebt(address(connector));
console.log("coll1 :", coll1);
console.log("debt1 :", debt1);
console.log("minHF :", connector.minimumHealthFactor()); // 1.5
uint256 connectorMinHF = connector.minimumHealthFactor();
uint256 troveHF = tm.getCurrentICR(address(connector), wethPrice * 1e10); // or coll1 * (wethPrice * 1e10) / debt1;
console.log("troveHF :", troveHF);
assertGt(troveHF, connector.minimumHealthFactor()); // 2000 ULTRA is backed up by 2 WETH, so troveHF is well above 1.5
// Next call fails, when it shouldn't; health factor of the trove is above connector.minimumHealthFactor()
vm.expectRevert();
// repay 1 ULTRA
connector.adjustTrove(
IStakeNTroveZap(ULTRA_StakeNTroveZap),
ULTRA_weeth_troveManager,
125e14, // max fee
0, // collateral to withdraw
1e18, // repay amount
false // is repay
);
// There should be enough collateral to borrow 1 ULTRA, but call fails
vm.expectRevert();
// borrow 1 ULTRA
connector.adjustTrove(
IStakeNTroveZap(ULTRA_StakeNTroveZap),
ULTRA_weeth_troveManager,
125e14, // max fee
0, // collateral to withdraw
1e18, // borrow amount
true // is borrow
);
}Check the price for one unit of the collateral token using valueOracle. Then, call troveManager.getCurrentICR(address(this), collateralPrice) to get the actual CR and compare it to the minimum.
function adjustTrove(
IStakeNTroveZap zapContract,
address tm,
uint256 mFee,
uint256 wAmount,
uint256 bAmount,
bool isBorrowing
) public onlyManager nonReentrant {
bytes32 positionId =
registry.calculatePositionId(address(this), PRISMA_POSITION_ID, abi.encode(zapContract, tm));
if (registry.getHoldingPositionIndex(vaultId, positionId, address(this), "") == 0) {
revert IConnector_InvalidPosition(positionId);
}
IBorrowerOperations borrowerOps = zapContract.borrowerOps();
if (bAmount > 0 && !isBorrowing) {
_approveOperations(ITroveManager(tm).debtToken(), address(borrowerOps), bAmount);
}
borrowerOps.adjustTrove(tm, address(this), mFee, 0, wAmount, bAmount, isBorrowing, address(this), address(this));
_updateTokenInRegistry(ITroveManager(tm).debtToken());
// get health factor
- uint256 healthFactor = ITroveManager(tm).getNominalICR(address(this));
+ uint256 collateralPrice = _getValue(ITroveManager(tm).collateralToken(), ChainlinkOracleConnector.USD(), 1);
+ uint256 healthFactor = ITroveManager(tm).getCurrentICR(address(this), collateralPrice);
if (minimumHealthFactor > healthFactor) {
revert IConnector_LowHealthFactor(healthFactor);
}
emit AdjustTrove(address(zapContract), tm, mFee, wAmount, bAmount, isBorrowing);
}If ULTRA is repaid - don't revert if the collateral ratio is below minimum. This will allow repaying ULTRA even when below threshold, which will enable the protocol to repay the debt even in the edge case where Prisma CR is ok, but connectors' CR is below minimum.
In ITroveManager add the following two functions:
+ function getCurrentICR(address _borrower, uint256 _price) external view returns (uint256);
+ function collateralToken() external view returns (address);[H-18] In Dolomite, when opening a borrow position, the holding position in the Registry will never be updated due to the removePosition flag being set to true
Submitted by iamandreiski (View additional reports submitted by other wardens)
Whenever some of the connectors opens or closes a position, it is updated in the registry.
Usually if/when a position is closed (all asset amount withdrawn / borrow position closed in its entirety, etc.), the holding position is updated with the flag true for bool removePosition.
In Dolomite, whenever we open a new borrow position, the flag is also set to true which means that the position should be removed, and again when closing the borrow position, the flag is also set to true.
This will lead to that position never being accounted for in the Registry.
In Dolomite, whenever we want to open a borrow position, we can call the openBorrowPosition() function:
function openBorrowPosition(uint256 marketId, uint256 _amountWei, uint256 accountId)
public
onlyManager
nonReentrant
{
address token = dolomiteMargin.getMarketTokenAddress(marketId);
if (!registry.isTokenTrusted(vaultId, token, address(this))) {
revert IConnector_UntrustedToken(token);
}
// borrow
borrowPositionProxy.openBorrowPosition(
0, accountId, marketId, _amountWei, AccountBalanceHelper.BalanceCheckFlag.None
);
registry.updateHoldingPosition(
vaultId, registry.calculatePositionId(address(this), DOL_POSITION_ID, ""), abi.encode(accountId), "", true
);
}
As we can see, when calling the registry.updateHoldingPosition the removePosition flag is set to true (the last argument in the function call).
Usually this is done when we want to either close a position or we've emptied the market/pool (our balance/share is 0) and we're closing it, here is an example from the same connector Dolomite properly utilizing this in the withdraw function:
function withdraw(uint256 marketId, uint256 _amount) public onlyManager nonReentrant {
address token = dolomiteMargin.getMarketTokenAddress(marketId);
depositWithdrawalProxy.withdrawWeiFromDefaultAccount(
marketId, _amount, AccountBalanceHelper.BalanceCheckFlag.None
);
// Update token
_updateTokenInRegistry(token);
(uint256[] memory markets,,,) = dolomiteMargin.getAccountBalances(Info(address(this), 0));
if (markets.length == 0) {
registry.updateHoldingPosition(
vaultId, registry.calculatePositionId(address(this), DOL_POSITION_ID, ""), abi.encode(0), "", true
);
}
}
We can also see that when closing a borrow position in the same connector, the flag is also set to true:
function closeBorrowPosition(uint256[] memory marketIds, uint256 accountId) public onlyManager nonReentrant {
// repay
borrowPositionProxy.closeBorrowPosition(accountId, 0, marketIds);
registry.updateHoldingPosition(
vaultId, registry.calculatePositionId(address(this), DOL_POSITION_ID, ""), abi.encode(accountId), "", true
);
}
Due to the following circuit-breaker check, the borrow position would never be updated as part of the holdingPositions array:
if (positionIndex == 0 && removePosition) return type(uint256).max;
The positionIndex is based on the isPositionUsed mapping:
bytes32 holdingPositionId = keccak256(abi.encode(msg.sender, _positionId, _data));
uint256 positionIndex = vault.isPositionUsed[holdingPositionId];
Since that mapping value of the holdingPositionId would have never been initiated, due to the circuit-breaker check above returning type(uint256).max, it would be 0, thus interrupting the function flow.
If the flag hasn't been set to true, the function flow would continue to the end, calling the other updateHoldingPosition function, and updating the isPositionUsed mapping:
return
updateHoldingPosition(vault, vaultId, _positionId, _data, additionalData, positionIndex, holdingPositionId);
This position would have never been accounted for in the Registry, and if the strategy's actions are based on Registry data, it can fail to close the position in-time, possibly leading to liquidations and bad debt.
Set the removePosition flag on the openBorrowPosition when calling the updateHoldingPosition function to false.
gzeon (judge) increased severity to High and commented:
Both open and close borrow are impacted.
Submitted by Zac (View additional reports submitted by other wardens)
There are numerous issues when calculating the TVL for the MorphoBlue connector
-
decimals are not considered in the function
convertCToLthis means that if the loanToken and collateralToken have a different number of decimals the TVL calculation will be wrong. -
borrowAmount is added rather than subtracted when calculating the TVL. This means that any borrowing will increase the TVL and any repaying will decrease the TVL
-
When withdrawing the function
_updateTokenInRegistryis only called for the collateralToken and not the loanToken. This means that if the loanToken is withdrawn the TVL will be less than it should be.
If the TVL is incorrect, users will recieve an incorrect number of shares when depositing and an incorrect number of base tokens when withdrawing. The below table shows the possible scenarios.
| User | TVL lower than expected | TVL higher than expected |
|---|---|---|
| Deposits | Recieves more shares than should be entitled to | Recieves fewer base tokens than should be entitled to |
| Withdraws | Recieves less shares than should be entitled to | Recieves more base tokens than should be entitled to |
The below test can be added to MorphoBlue.t.sol.
It shows each of the 3 points above causing the TVL calculation to be incorrect.
function testSupplyBorrowRepayWithdraw() public {
uint256 amount = 1000 * 1e6;
uint256 amount_dai = 1000 * 1e18;
console.log("----------- Test Deposit -----------");
_dealWhale(baseToken, address(connector), USDC_Whale, amount);
_dealERC20(DAI, address(connector), amount_dai);
vm.startPrank(owner);
assertEq(
address(morphoBlueContract), address(connector.morphoBlue()), "MorphoBlue address is not set correctly"
);
connector.supply(amount, usdcDaiMarketId, true);
uint256 tvl = accountingManager.TVL();
console.log("TVL: %s", tvl); // tvl = 1000000000
connector.supply(amount_dai, usdcDaiMarketId, false);
tvl = accountingManager.TVL();
console.log("TVL before borrow: %s", tvl); // tvl = 1000000000001000000000
// NOTE: Due to DAI having a different number of decimals than USDC, the TVL increases by a very large amount
// correct TVL = 2000000000
connector.borrow(amount / 2, usdcDaiMarketId);
tvl = accountingManager.TVL();
console.log("TVL after borrow: %s", tvl); // 1000000000002000000000
// NOTE: The TVL increases because the borrow is added instead of subtracted from the TVL
connector.repay(amount / 2, usdcDaiMarketId);
tvl = accountingManager.TVL();
console.log("TVL after repay: %s", tvl); // 1000000000001000000000
connector.withdraw(amount, usdcDaiMarketId, true);
tvl = accountingManager.TVL();
console.log("TVL after withdraw 1: %s", tvl); // 1000000000000000000000
connector.withdraw(amount_dai, usdcDaiMarketId, false);
tvl = accountingManager.TVL();
console.log("TVL after withdraw 2: %s", tvl); // 999945369
// NOTE: The TVL is half of what it should be because the withdraw function does not call _updateTokenInRegistry for the loanToken
vm.stopPrank();
}
Manual Review, Foundry
Each of the 3 points above must be fixed individually for the TVL calculation to be correct. Each issue can be fixed as follows:
- For the function
convertCToLthe result should be divided by 10**(collateralTokenDecimals) and multiplied by 10**(loanTokenDecimals). - The borrow amount should be subtracted rather than added in the
_getPositionTVLfunction. https://github.com/code-423n4/2024-04-noya/blob/9c79b332eff82011dcfa1e8fd51bad805159d758/contracts/connectors/MorphoBlueConnector.sol#L132 - call
_updateTokenInRegistryfor the loanToken in thewithdrawfunction. https://github.com/code-423n4/2024-04-noya/blob/9c79b332eff82011dcfa1e8fd51bad805159d758/contracts/connectors/MorphoBlueConnector.sol#L58
[H-20] _getPositionTVL of UNIv3Connector wrongly assumes ownership of all liquidity of the provided ticks inside positionManager
Submitted by said, also found by SBSecurity, BenRai, and SeveritySquad
When _getPositionTVL is called, it queries position information from the pool using the calculated key. However, it assumes that all liquidity within the provided ticks in the positionManager is owned by the connector.
When _getPositionTVL is called, it calculate key by providing positionManager, tL, tU. Then used the key to get liquidity information inside the pool.
function _getPositionTVL(HoldingPI memory p, address base) public view override returns (uint256 tvl) {
PositionBP memory positionInfo = registry.getPositionBP(vaultId, p.positionId);
uint256 tokenId = abi.decode(p.data, (uint256));
(address token0, address token1) = abi.decode(positionInfo.data, (address, address));
uint256 amount0;
uint256 amount1;
(int24 tL, int24 tU, uint24 fee) = abi.decode(p.additionalData, (int24, int24, uint24));
{
IUniswapV3Pool pool = IUniswapV3Pool(factory.getPool(token0, token1, fee));
>>> bytes32 key = keccak256(abi.encodePacked(positionManager, tL, tU));
>>> (uint128 liquidity,,, uint128 tokensOwed0, uint128 tokensOwed1) = pool.positions(key);
(uint160 sqrtPriceX96,,,,,,) = pool.slot0();
(amount0, amount1) = LiquidityAmounts.getAmountsForLiquidity(
sqrtPriceX96, TickMath.getSqrtRatioAtTick(tL), TickMath.getSqrtRatioAtTick(tU), liquidity
);
amount0 += tokensOwed0;
amount1 += tokensOwed1;
}
tvl += valueOracle.getValue(token0, base, amount0);
tvl += valueOracle.getValue(token1, base, amount1);
}This assumption is incorrect as it presumes that all liquidity within the provided ticks is owned by Noya's UniV3 Connector. This leads to wrong TVL calculation within the protocol, potentially inflating the TVL beyond its actual value. Consequently, this could undermine the protocol's solvency by incorrectly inflating its perceived total assets.
User positionsManager positions getter instead to provide accurate liquidity owner by connector.
HadiEsna (NOYA) confirmed and commented:
True, needs to be fixed. Not High severity though in my opinion.
[H-21] Decreasing a position in PendleConnector will remove it even if there's still a stake at Penpie
Submitted by Sentryx
https://github.com/code-423n4/2024-04-noya/blob/main/contracts/connectors/PendleConnector.sol#L303-L309
https://github.com/code-423n4/2024-04-noya/blob/main/contracts/connectors/PendleConnector.sol#L223-L231
decreasePosition() will falsely remove the market as a holding position, ignoring the staked Pendle market LP tokens at Penpie and thus that stake won't be counted towards the TVL of the connector and the TVL of the AccountingManager the connector belongs to.
The PendleConnector has a function decreasePosition() to redeem Pendle SY tokens in exchange for underlying tokens with which it effectively would remove its stake in a given Pendle market.
function decreasePosition(IPMarket market, uint256 _amount, bool closePosition) external onlyManager nonReentrant {
(IPStandardizedYield SY,,) = market.readTokens();
(, address _underlyingToken,) = SY.assetInfo();
// redeems an amount of base tokens by burning SY
IERC20(address(SY)).safeTransfer(address(SY), _amount);
IPStandardizedYield(address(SY)).redeem(address(this), _amount, _underlyingToken, 1, true);
→ if (closePosition && isMarketEmpty(market)) {
registry.updateHoldingPosition(
vaultId,
registry.calculatePositionId(address(this), PENDLE_POSITION_ID, abi.encode(market)),
"",
"",
true
);
}
emit DecreasePosition(address(market), _amount, closePosition);
}When the closePosition parameter is true, isMarketEmpty() would be called for the given market to check if the connector holds any of the 3 tokens composing it - Yield token (YT), Principal token (PT) and Standardized Yield token (SY).
function isMarketEmpty(IPMarket market) public view returns (bool) {
(IPStandardizedYield _SY, IPPrincipalToken _PT, IPYieldToken _YT) = IPMarket(market).readTokens();
return (
_SY.balanceOf(address(this)) == 0 && _PT.balanceOf(address(this)) == 0 && _YT.balanceOf(address(this)) == 0
&& market.balanceOf(address(this)) == 0
);
}The function however does not check if the connector still has a balance in the Penpie pool for this market (a stake), and that will allow the connector to remove its holding position for a market, effectively excluding its stake in the Penpie pool from the connector and the AccountingManager TVL.
In the isMarketOpen() function check for the connector's balance in the Penpie staking pool for this market.
diff --git a/contracts/connectors/PendleConnector.sol b/contracts/connectors/PendleConnector.sol
index 17607ee..e1b1208 100644
--- a/contracts/connectors/PendleConnector.sol
+++ b/contracts/connectors/PendleConnector.sol
@@ -304,7 +304,7 @@ contract PendleConnector is BaseConnector {
(IPStandardizedYield _SY, IPPrincipalToken _PT, IPYieldToken _YT) = IPMarket(market).readTokens();
return (
_SY.balanceOf(address(this)) == 0 && _PT.balanceOf(address(this)) == 0 && _YT.balanceOf(address(this)) == 0
- && market.balanceOf(address(this)) == 0
+ && market.balanceOf(address(this)) == 0 && pendleMarketDepositHelper.balance(market, address(this)) == 0
);
}
gzeon (judge) increased severity to High
Submitted by 0xAlix2
Pendle connector allows protocol managers to deposit some underlying asset in return for some SY tokens, which can be used to mint PT and YT tokens. PT and YT can be deposited in markets and in return for LP tokens and some claimable rewards. These LP tokens should be considered when calculating the TVL of the position, to do this the protocol is using getLpToAssetRate, which from the docs:
getLpToAssetRate: Serves the same purpose, but in terms of SY's asset.
This means that it returns the price of LP in terms of the underlying asset (which is USDT in the below example); however, the protocol is using this function assuming that it returns the price of LP in terms of SY, through:
SYAmount += lpBalance * IPMarket(market).getLpToAssetRate(10) / 1e18;
The above also applies to getPtToAssetRate.
This will cause an inaccurate representation of the Connector's TVL affecting the whole protocol's TVL.
function testInvalidPendleTVL() public {
(IPStandardizedYield _SY, IPPrincipalToken _PT, IPYieldToken _YT) = IPMarket(pendleUsdtMarket).readTokens();
uint256 USDTamount = 100e6;
_dealERC20(USDT, address(connector), USDTamount);
vm.startPrank(owner);
// Update tokens in registry
connector.updateTokenInRegistry(USDT);
// TVL is around 100 USDC
// Connector holds 100 USDT
assertEq(accountingManager.TVL() / 1e6, 99);
assertEq(IERC20(USDT).balanceOf(address(connector)) / 1e6, 100);
// Supply USDT to the market
connector.supply(pendleUsdtMarket, USDTamount);
// TVL is still around 100 USDC
// Connector holds 0 USDT and around 100 SY
assertEq(accountingManager.TVL() / 1e6, 99);
assertEq(IERC20(USDT).balanceOf(address(connector)), 0);
assertEq(_SY.balanceOf(address(connector)) / 1e6, 99);
// Mint PT and YT
connector.mintPTAndYT(pendleUsdtMarket, _SY.balanceOf(address(connector)) / 2);
// TVL is still around 100 USDC
// Connector holds 0 USDT, 49 PT, 49 YT and 49 SY
assertEq(accountingManager.TVL() / 1e6, 100);
assertEq(IERC20(USDT).balanceOf(address(connector)), 0);
assertEq(_SY.balanceOf(address(connector)) / 1e6, 49);
assertEq(_PT.balanceOf(address(connector)) / 1e6, 49);
assertEq(_YT.balanceOf(address(connector)) / 1e6, 49);
// Deposit SY and PT into the market
connector.depositIntoMarket(
IPMarket(pendleUsdtMarket), _SY.balanceOf(address(connector)), _PT.balanceOf(address(connector))
);
vm.roll(block.number + 20);
vm.warp(block.timestamp + 1 hours);
// TVL drops to 59 USDC - which is wrong because of the invalid TVL calculation
// Connector holds 0 USDT, 49 YT, 29 LP, 0 SY and 0 PT
assertEq(accountingManager.TVL() / 1e6, 59);
assertEq(IERC20(_YT).balanceOf(address(connector)) / 1e6, 49);
assertEq(IERC20(pendleUsdtMarket).balanceOf(address(connector)) / 1e6, 29);
assertEq(_SY.balanceOf(address(connector)), 0);
assertEq(_PT.balanceOf(address(connector)), 0);
}
Refactor PendleConnector::_getPositionTVL to take into consideration that getLpToAssetRate and getPtToAssetRate calculate the price of LP in terms of the underlying asset and not SY.
gzeon (judge) increased severity to High
Submitted by 0xAlix2, also found by bigtone and BenRai
In Dolomite protocol, transferBetweenAccounts transfers the collateral from 1 account to another under the same owner. So the owner of both accounts still owns these funds regardless of the account they're deposited in. In DolomiteConnector, when calling transferBetweenAccounts the protocol isn't updating the holding position of either account.
So when the manager moves the collateral to account ID 1 for example, the protocol doesn't know about the new holding position (which is the position related to account ID 1) and will remain calculating the TVL according to the old holding position (account Id 0), even if the whole amount of account 0 was moved to account 1.
This will cause an inaccurate representation of the Connector's TVL affecting the whole protocol's TVL.
function testDolomiteInvalidHoldingPositions_transferBetweenAccounts() public {
uint256 marketId = 1;
uint256 defaultAccountId = 0;
uint256 accountId = 1;
uint256 amount = 200e18;
_dealERC20(DAI, address(connector), amount);
vm.startPrank(owner);
// Deposits 200 DAI to Dolomite
connector.deposit(marketId, amount);
// TVL is around 200 Base Tokens
assertEq(accountingManager.TVL() / 1e6, 199);
// Transfer collateral from account 0 to account 1
connector.transferBetweenAccounts(accountId, marketId, amount, false);
// TVL has dropped to 0
assertEq(accountingManager.TVL(), 0);
// Connector's account 0 has no markets (no collateral left)
(uint256[] memory markets,,,) =
IDolomiteMargin(dolomiteMargin).getAccountBalances(Info(address(connector), defaultAccountId));
assertEq(markets.length, 0);
// Connector's account 1 has 1 market (some collateral exists)
(markets,,,) = IDolomiteMargin(dolomiteMargin).getAccountBalances(Info(address(connector), accountId));
assertEq(markets.length, 1);
}
Add the following at the bottom of DolomiteConnector::transferBetweenAccounts:
(uint256[] memory markets0,,,) = dolomiteMargin.getAccountBalances(Info(address(this), 0));
registry.updateHoldingPosition(
vaultId,
registry.calculatePositionId(address(this), DOL_POSITION_ID, ""),
abi.encode(0),
"",
markets0.length == 0
);
(uint256[] memory markets1,,,) = dolomiteMargin.getAccountBalances(Info(address(this), accountId));
registry.updateHoldingPosition(
vaultId,
registry.calculatePositionId(address(this), DOL_POSITION_ID, ""),
abi.encode(accountId),
"",
markets1.length == 0
);
This updates the holding positions of both accounts, by either setting or removing the position according to the open markets of each.
gzeon (judge) increased severity to High
Note: full content and discussions can be accessed by viewing each linked submission.
Submitted by MrPotatoMagic, also found by BenRai
Status: Sponsor confirmed
Submitted by WinSec (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-03] Noya is not compatible with tokens whose balance changes outside of transfers, causing funds to get stuck in the contract
Submitted by Audinarey (View additional reports submitted by other wardens)
Status: Sponsor disputed
[M-04] performanceFeeReceiver cannot mint any performance fee shares even if TVL is dropped by only a very tiny amount
Submitted by rbserver (View additional reports submitted by other wardens)
Status: Sponsor disputed
[M-05] AccountingManager contract's previewDeposit, previewMint, previewWithdraw, and previewRedeem functions are not compliant with EIP-4626 standard
Submitted by rbserver (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by rbserver (View additional reports submitted by other wardens)
Status: Sponsor acknowledged
Submitted by rbserver (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by cheatc0d3 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by sh1v (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by rbserver (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by TPSec (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by ArsenLupin (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by honey-k12 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-14] In the BalancerConnector, unclaimed rewards are not included in the calculation of the connectors TVL
Submitted by BenRai (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-15] PendleConnector::_getPositionTVL will revert in the current implementation because there is no need to stake the LP tokens anymore
Submitted by BenRai
Status: Sponsor disputed
[M-16] borrowAndSupply() and withdraw() of FraxConnector should not be blocked when maxLTV of the Frax pair is 0
Submitted by KupiaSec
Status: Sponsor confirmed
[M-17] Incorrect Return Value in CompoundConnector.getBorrowBalanceInBase() Affecting TVL Calculation
Submitted by KupiaSec (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-18] AccountingManager has no correct implementations of the core ERC-4626 functions deposit, mint, withdraw and redeem
Submitted by d3e4 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-19] Attacker can increase the length of withdrawQueue by withdrawing 0 amount of tokens frequently
Submitted by Weed0607 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-20] totalAssets(), and thus convertToShares() and convertToAssets(), may revert, in violation of ERC-4626
Submitted by d3e4 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by grearlake (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-22] In the SNXV3Connector, unclaimed rewards are not included in the calculation of the connectors TVL
Submitted by BenRai
Status: Sponsor confirmed
Submitted by Weed0607 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by alexxander (View additional reports submitted by other wardens)
Status: Sponsor acknowledged
[M-25] Base tokens accumulated from withdraw fees can't be transferred to/from the NoyaFeeReceiver and will remain stuck
Submitted by iamandreiski, also found by SBSecurity
Status: Sponsor disputed
[M-26] The TVLHelper.sol#getTVL function is DOSed by the under collateralized connector, and as a result, many parts of the protocol may be DOS
Submitted by FastChecker (View additional reports submitted by other wardens)
Status: Sponsor disputed
Submitted by WinSec (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-28] LP tokens from Boosted Positions are not included in the TVL calculation of a position held by the MaverickConnector
Submitted by BenRai (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by SeveritySquad (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-30] CompoundConnector.sol misses unclaimed rewards in getPositionTVL, resulting in undervalued positionTVL/TVL
Submitted by oakcobalt (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by SeveritySquad (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by pkqs90, also found by BenRai
Status: Sponsor confirmed
[M-33] FraxConnector.sol#repay approved amount of tokens does NOT account for latest interest, which may cause DoS while repaying
Submitted by pkqs90
Status: Sponsor confirmed
Submitted by pkqs90, also found by 0xAlix2
Status: Sponsor confirmed
[M-35] AccountingManager#totalWithdrawnAmount should reflect tokens actually transferred to users, instead of expected transfers
Submitted by pkqs90 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-36] veMav token in MaverickConnector does NOT have an existing oracle, so staking Mav would always lead to DoS for TVL calculation
Submitted by pkqs90 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by JanuaryPersimmon2024 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-38] PendlingConnector::depositIntoMarket() PendlingConnector::burnLP() and are missing slippage control parameters
Submitted by ladboy233 (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by ladboy233 (View additional reports submitted by other wardens)
Status: Sponsor disputed
[M-40] Dust donation might DOS all connectors to create new holding positions, by preventing removing existing holding positions
Submitted by oakcobalt (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-41] A malicious user can front-run Morpho repay with a small repay amount making the transaction revert for underflow, DoSing the repay
Submitted by KYP
Status: Sponsor confirmed
Submitted by KYP (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by Tigerfrake, also found by 0xAlix2
Status: Sponsor confirmed
Submitted by DPS (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by joaovwfreire, also found by joaovwfreire
Status: Sponsor confirmed
[M-46] The watchers cannot perform their role and can't do anything to intervene during bridging as stated by the docs
Submitted by SeveritySquad
Status: Sponsor confirmed
[M-47] The health factor check in PrismaConnector::adjustTrove will always pass because the ICR in the Primsma protocol has 20 decimals
Submitted by BenRai, also found by ArsenLupin
Status: Sponsor confirmed
Submitted by said (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-49] Lack of functionality for claimFees calls to the Aerodrome Pool causes the connector to lose its deserved fees
Submitted by said, also found by pkqs90
Status: Sponsor confirmed
[M-50] Due to missing health factor and hardcoded balance checks on Dolomite, a borrow position can be opened by withdrawing more than the supplied balance leading to possible unwanted liquidations
Submitted by iamandreiski (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by Sentryx (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-52] Some connectors prevent repayment of a borrow position if it doesn't leave the connector solvent or above minimumHealthFactor
Submitted by Sentryx (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by SpicyMeatball (View additional reports submitted by other wardens)
Status: Sponsor acknowledged
[M-54] Maverick Connector uses ETH as liquidity for some of the Maverick pools, but NOYA isn't equipped to handle ETH in its vaults without handling conversion to/from WETH in the connector
Submitted by iamandreiski
Status: Sponsor confirmed
[M-55] In the AerodromeConnector, unclaimed rewards are not included in the calculation of the connectors TVL
Submitted by BenRai (View additional reports submitted by other wardens)
Status: Sponsor confirmed
Submitted by BenRai (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-57] Burning sUSD in SNXV3Connector:: mintOrBurnSUSD will not work because the sUSD to burn are not deposited into the SNXV3 protocol
Submitted by BenRai
Status: Sponsor confirmed
Submitted by BenRai
Status: Sponsor confirmed
[M-59] If a curve pool which CurveConnector uses is killed, the vault manager can't close the position, leading to loss of funds
Submitted by iamandreiski (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-60] FullMath library is missing unchecked blocks, leading to DOS protocol's TVL and UniswapValueOracle
Submitted by 0xAlix2, also found by ZanyBonzy
Status: Sponsor confirmed
[M-61] lzSend() forwards all of the contract balance as the native gas fee but the excess won't be always returned
Submitted by iamandreiski
Status: Sponsor confirmed
[M-62] CurveConnector will be non-functional on Arbitrum & Polygon due to the improper integration with Convex Boosters on these chains
Submitted by Bauchibred (View additional reports submitted by other wardens)
Status: Sponsor confirmed
[M-63] The check when increasing the minimumHelthFactor in the SiloConnector is wrong because this variable is used differently in this connector
Submitted by BenRai
Status: Sponsor confirmed
For this audit, 23 reports were submitted by wardens detailing low risk and non-critical issues. This report submitted by Bauchibred received the top score from the judge.
C4 is an open organization governed by participants in the community.
C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.