Platform: Code4rena
Start Date: 10/11/2023
Pot Size: $28,000 USDC
Total HM: 5
Participants: 185
Period: 5 days
Judge: 0xDjango
Id: 305
League: ETH
Rank: 3/185
Findings: 5
Award: $1,086.27
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: m_Rassska
Also found by: 0xDING99YA, Aamir, Bauchibred, CatsSecurity, HChang26, PENGUN, SBSecurity, adriro, almurhasan, anarcheuz, circlelooper, d3e4, deth, jayfromthe13th
902.5718 USDC - $902.57
Prices returned from Chainlink oracles have different conditions to update the reported values, which can be abused by
Prices for the different LST assets supported in the Kelp protocol are obtained from a Chainlink oracle. The data feeds for each one can be found here:
Chainlink price feeds follow different rules in order to update their prices. There are essentially two conditions that could trigger a price change, a heartbeat and a price deviation. A heartbeat updates the feed if the configured time since last update has elapsed, and the price deviation triggers the update when the price has moved above a configured percentage.
In all three cases, the heartbeat is the same (24 hours), however the price deviation target is not. stETH has a percentage of 0.5%, cbETH of 1% and rETH of 2%. This means that there are big differences between how fast an oracle is updated between the different assets.
As deposits in the protocol can be done using any assets, this means that a user could choose to deposit a particular asset knowing that the current on-chain price is deviated from the actual price, which can go as far as 2% for the rETH case. Note also that this can be used in a front-run fashion before the price of an oracle is updated on-chain, reducing the uncertainty to almost none in order to take advantage of a big price movement.
Similarly, since feeds have different triggers, a user could use this to their advantage to deposit the asset that is currently most deviated between all three. As participation in the protocol is measured by a share of RSETH, this means that the user's share will also include the other two assets, gaining an advantage in value due to the mispriced deposit asset.
Currently, there is no way to withdraw assets from the protocol, but this could eventually lead to a situation where a user deposits one asset before the price movement, and then withdraws after the price has impacted, or simply withdraws any other asset from the pool which has a more recent price update.
As the protocol is currently relying on Chainlink oracles, it is difficult to provide a recommendation using solely these price feeds. A potential solution would be to factor different oracles, such as a TWAP or other on-chain price sources.
Oracle
#0 - c4-pre-sort
2023-11-16T23:36:28Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-11-16T23:36:55Z
raymondfam marked the issue as duplicate of #609
#2 - c4-judge
2023-12-01T17:43:14Z
fatherGoose1 marked the issue as unsatisfactory: Invalid
#3 - romeroadrian
2023-12-03T23:59:43Z
@fatherGoose1 I really don't think this issue is correctly marked as a duplicate of #609. I think this is more on the line of #584.
#4 - c4-judge
2023-12-08T17:32:46Z
fatherGoose1 marked the issue as duplicate of #584
#5 - c4-judge
2023-12-08T17:50:31Z
fatherGoose1 marked the issue as satisfactory
#6 - c4-judge
2023-12-08T18:55:04Z
fatherGoose1 changed the severity to 3 (High Risk)
#7 - d3e4
2023-12-08T21:45:10Z
@fatherGoose1 This report states that the price oracle might be inaccurate. This is not sufficient for #584. The only thing even related to #584 is "As participation in the protocol is measured by a share of RSETH, this means that the user's share will also include the other two assets, gaining an advantage in value due to the mispriced deposit asset." There is not enough information here to arrive at the issue in #584.
"Currently, there is no way to withdraw assets from the protocol, but this could eventually lead to a situation where a user deposits one asset before the price movement, and then withdraws after the price has impacted, or simply withdraws any other asset from the pool which has a more recent price update." seems to suggest the issue described in #443, which is about the fact that you can predict the market at the moment of a price update, but unrelated to #584
#8 - romeroadrian
2023-12-08T21:51:15Z
@fatherGoose1 This report states that the price oracle might be inaccurate. This is not sufficient for #584. The only thing even related to #584 is "As participation in the protocol is measured by a share of RSETH, this means that the user's share will also include the other two assets, gaining an advantage in value due to the mispriced deposit asset." There is not enough information here to arrive at the issue in #584.
"Currently, there is no way to withdraw assets from the protocol, but this could eventually lead to a situation where a user deposits one asset before the price movement, and then withdraws after the price has impacted, or simply withdraws any other asset from the pool which has a more recent price update." seems to suggest the issue described in #443, which is about the fact that you can predict the market at the moment of a price update, but unrelated to #584
Nice to see you here d, I'm clearly not stating that the price oracle might be inaccurate, I'd recommend reading the issue again.
#9 - d3e4
2023-12-08T22:35:12Z
@romeroadrian What is your point? Do you mean that what you stated is contrary to "the price oracle might be inaccurate" (in which case you are wrong) or that you stated something else (what?)?
#10 - romeroadrian
2023-12-08T23:13:24Z
@romeroadrian What is your point? Do you mean that what you stated is contrary to "the price oracle might be inaccurate" (in which case you are wrong) or that you stated something else (what?)?
I mean that you summarized my report using those words which is not even close to the reported issue. I encourage you to read it again.
#11 - d3e4
2023-12-08T23:37:38Z
@romeroadrian Please be specific about how you think I have misread your report. A dodging encouragement to read it again will not help anyone.
I did not summarise your report by those words. This was the first part of your report. I then mentioned the two other things in your report. If you think I missed something important, be explicit about what it is.
#12 - romeroadrian
2023-12-09T14:00:50Z
I did not summarise your report by those words. This was the first part of your report.
I think you are really confused here. Those are your words, I literally quoted them. That's not part of my report.
#13 - d3e4
2023-12-09T16:43:02Z
You are abusing a literal parsing of what I wrote. I am sure you understand what I meant.
Are you unable to be explicit and specific about what I missed?
🌟 Selected for report: T1MOH
Also found by: 0x1337, 0xNaN, 0xepley, 0xluckhu, 0xmystery, 7siech, Aamir, AlexCzm, Aymen0909, DanielArmstrong, GREY-HAWK-REACH, HChang26, Jiamin, Juntao, QiuhaoLi, Ruhum, SBSecurity, Varun_05, Weed0607, adam-idarrha, adriro, ast3ros, ayden, circlelooper, crack-the-kelp, crunch, cryptothemex, deepplus, mahdirostami, max10afternoon, osmanozdemir1, rouhsamad, rvierdiiev, trachev, xAriextz, zhaojie
36.0335 USDC - $36.03
The calculation in the deposit function of the DepositPool contract is flawed as it factors the deposited amount into the RSETH price to calculate the amount to mint.
When a user deposits in the DepositPool contract, the amount of RSETH to mint is determined by the price returned by the LRTOracle:
52: function getRSETHPrice() external view returns (uint256 rsETHPrice) { 53: address rsETHTokenAddress = lrtConfig.rsETH(); 54: uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply(); 55: 56: if (rsEthSupply == 0) { 57: return 1 ether; 58: } 59: 60: uint256 totalETHInPool; 61: address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL); 62: 63: address[] memory supportedAssets = lrtConfig.getSupportedAssetList(); 64: uint256 supportedAssetCount = supportedAssets.length; 65: 66: for (uint16 asset_idx; asset_idx < supportedAssetCount;) { 67: address asset = supportedAssets[asset_idx]; 68: uint256 assetER = getAssetPrice(asset); 69: 70: uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset); 71: totalETHInPool += totalAssetAmt * assetER; 72: 73: unchecked { 74: ++asset_idx; 75: } 76: } 77: 78: return totalETHInPool / rsEthSupply; 79: }
In summary, the implementation takes all assets held by the protocol and normalizes them into ETH. This value is then divided by the current supply of RSETH to get the price per token. For each asset, deposits are given by the getTotalAssetDeposits()
function:
47: function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) { 48: (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) = 49: getAssetDistributionData(asset); 50: return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer); 51: }
71: function getAssetDistributionData(address asset) 72: public 73: view 74: override 75: onlySupportedAsset(asset) 76: returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) 77: { 78: // Question: is here the right place to have this? Could it be in LRTConfig? 79: assetLyingInDepositPool = IERC20(asset).balanceOf(address(this)); 80: 81: uint256 ndcsCount = nodeDelegatorQueue.length; 82: for (uint256 i; i < ndcsCount;) { 83: assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]); 84: assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset); 85: unchecked { 86: ++i; 87: } 88: } 89: }
The implementation sums the assets owned by the DepositPool, as well as the balance and staked amount in Eigenlayer for each node delegator.
The RSETH price is then used to calculate the minting amount in depositAsset()
:
119: function depositAsset( 120: address asset, 121: uint256 depositAmount 122: ) 123: external 124: whenNotPaused 125: nonReentrant 126: onlySupportedAsset(asset) 127: { 128: // checks 129: if (depositAmount == 0) { 130: revert InvalidAmount(); 131: } 132: if (depositAmount > getAssetCurrentLimit(asset)) { 133: revert MaximumDepositLimitReached(); 134: } 135: 136: if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) { 137: revert TokenTransferFailed(); 138: } 139: 140: // interactions 141: uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount); 142: 143: emit AssetDeposit(asset, depositAmount, rsethAmountMinted); 144: }
151: function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) { 152: (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount); 153: 154: address rsethToken = lrtConfig.rsETH(); 155: // mint rseth for user 156: IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint); 157: }
095: function getRsETHAmountToMint( 096: address asset, 097: uint256 amount 098: ) 099: public 100: view 101: override 102: returns (uint256 rsethAmountToMint) 103: { 104: // setup oracle contract 105: address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE); 106: ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress); 107: 108: // calculate rseth amount to mint based on asset amount and asset exchange rate 109: rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice(); 110: }
getRsETHAmountToMint()
takes the deposit amount, multiplies it by the asset price (to normalize it to ETH) and divides it by the previously mentioned RSETH price calculation. However, the implementation of depositAsset()
transfers first the deposit into the contract before fetching the current price. Line 136 pulls the tokens from the user into the contract and the call in line 141 does the calculation. This means the RSETH price calculation, which is based on the amount of assets held by the protocol (including the ones in the DepositPool), will be affected by the same transfer that happens in the function.
As balances are increased by the user's tokens, the RSETH price will be increased too (since the supply is still the same), which means that the user will be minted less RSETH tokens than expected.
In the following test, Alice first deposits 1e18
stETH tokens. As this is the first deposit in the pool, she is minted 1e18
shares of RSETH. Now, Bob deposits the same amount of stETH tokens but he just gets 0.5e18
RSETH tokens, half of what Alice received for depositing the same amount.
The difference comes from the fact that Bob's tokens are pulled into the contract before calculating the price, effectively duplicating the price of RSETH. Once the price is doubled, Bob is minted half of what it should be.
Note: the snippet shows only the relevant code for the test. Full test file can be found here.
function test_DepositPool_WrongMintCalculation() public { uint256 depositAmount = 1e18; // Alice deposits 1e18 stETH vm.startPrank(alice); stETH.approve(address(lrtDepositPool), type(uint256).max); lrtDepositPool.depositAsset(address(stETH), depositAmount); vm.stopPrank(); // Alice is minted 1e18 RSETH assertEq(rseth.balanceOf(alice), 1e18); // Now, Bob deposits the same amount vm.startPrank(bob); stETH.approve(address(lrtDepositPool), type(uint256).max); lrtDepositPool.depositAsset(address(stETH), depositAmount); vm.stopPrank(); // Bob is minted just 0.5e18 RSETH when he deposited the same amount as Alice assertEq(rseth.balanceOf(bob), 0.5e18); }
The RSETH price calculation should not factor the tokens that are part of the deposit. Either, snapshot the price before pulling the tokens, or simply switch the minting order so that tokens are pulled after the mint:
+ // interactions + uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount); if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) { revert TokenTransferFailed(); } - // interactions - uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount); emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
Other
#0 - c4-pre-sort
2023-11-16T23:31:58Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-11-16T23:32:08Z
raymondfam marked the issue as duplicate of #62
#2 - c4-judge
2023-11-29T21:26:12Z
fatherGoose1 marked the issue as satisfactory
#3 - c4-judge
2023-12-01T19:00:05Z
fatherGoose1 changed the severity to 2 (Med Risk)
#4 - c4-judge
2023-12-04T15:31:41Z
fatherGoose1 changed the severity to 3 (High Risk)
🌟 Selected for report: Krace
Also found by: 0xDING99YA, 0xrugpull_detector, Aamir, AlexCzm, Aymen0909, Banditx0x, Bauer, CatsSecurity, GREY-HAWK-REACH, Madalad, Phantasmagoria, QiuhaoLi, Ruhum, SBSecurity, SandNallani, SpicyMeatball, T1MOH, TheSchnilch, adam-idarrha, adriro, almurhasan, ast3ros, ayden, bronze_pickaxe, btk, chaduke, ck, crack-the-kelp, critical-or-high, deth, gumgumzum, jasonxiale, joaovwfreire, ke1caM, m_Rassska, mahdirostami, mahyar, max10afternoon, osmanozdemir1, peanuts, pep7siup, peter, ptsanev, qpzm, rouhsamad, rvierdiiev, spark, twcctop, ubl4nk, wisdomn_, zach, zhaojie
4.6614 USDC - $4.66
The DepositPool contract is susceptible to the Inflation Attack, in which the first depositor can be front-runned by an attacker to steal their deposit.
The DepositPool pool contract acts mainly as a vault: accounts deposit LST assets and get back RSETH tokens (a share) that represents their participation in the vault.
These types of contracts are potentially vulnerable to the inflation attack: an attacker can front-run the initial deposit to the vault to inflate the value of a share and render the front-runned deposit worthless.
The mint amount in the pool is given by the getRsETHAmountToMint()
function:
095: function getRsETHAmountToMint( 096: address asset, 097: uint256 amount 098: ) 099: public 100: view 101: override 102: returns (uint256 rsethAmountToMint) 103: { 104: // setup oracle contract 105: address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE); 106: ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress); 107: 108: // calculate rseth amount to mint based on asset amount and asset exchange rate 109: rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice(); 110: }
While the getRSETHPrice()
is determined by the asset balance in the contracts:
52: function getRSETHPrice() external view returns (uint256 rsETHPrice) { 53: address rsETHTokenAddress = lrtConfig.rsETH(); 54: uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply(); 55: 56: if (rsEthSupply == 0) { 57: return 1 ether; 58: } 59: 60: uint256 totalETHInPool; 61: address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL); 62: 63: address[] memory supportedAssets = lrtConfig.getSupportedAssetList(); 64: uint256 supportedAssetCount = supportedAssets.length; 65: 66: for (uint16 asset_idx; asset_idx < supportedAssetCount;) { 67: address asset = supportedAssets[asset_idx]; 68: uint256 assetER = getAssetPrice(asset); 69: 70: uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset); 71: totalETHInPool += totalAssetAmt * assetER; 72: 73: unchecked { 74: ++asset_idx; 75: } 76: } 77: 78: return totalETHInPool / rsEthSupply; 79: }
An attacker can then mint a minimal amount of shares by depositing just 1 token of any valid asset, and then inflate the price per share by donating assets to the deposit pool. Once the share price has been inflated, the front-runned deposit is rounded down to zero or a very small number of shares, causing a loss of funds to the depositor as any precision loss will be captured by the attacker's shares.
For simplicity, let's say that the current Chainlink stETH/ETH price is 1 (1e18
).
(amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice() = 1 * 1e18 / 1e18 = 1
RSETH.X + 1
.lrtOracle.getRSETHPrice()
will be calculated using the sum of balances of stETH (there are no other assets in the deposit pool), (X + 1) * 1e18 / 1 = (X + 1) * 1e18
. The minted amount is then calculated as X * 1e18 / (X + 1) * 1e18 = X / (X + 1) = 0
. The user is minted 0 RSETH and the deposited amount belongs to the attacker's share.There are multiple ways to prevent the inflation attack:
There are multiple ways of solving the issue:
A very good discussion of these can be found here.
Other
#0 - c4-pre-sort
2023-11-16T23:33:15Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-11-16T23:33:22Z
raymondfam marked the issue as duplicate of #42
#2 - c4-judge
2023-12-01T17:02:48Z
fatherGoose1 changed the severity to 3 (High Risk)
#3 - c4-judge
2023-12-01T17:06:41Z
fatherGoose1 marked the issue as satisfactory
🌟 Selected for report: max10afternoon
Also found by: 0xMilenov, 0xbrett8571, 0xhacksmithh, 0xmystery, Aymen0909, Bauer, Daniel526, PENGUN, Pechenite, Shaheen, adriro, anarcheuz, btk, ck, ge6a, glcanvas, hals, turvy_fuzz
140.2525 USDC - $140.25
Users depositing in the protocol have no control over the amount of RSETH minted in return for their deposit.
The depositAsset()
function present in the LRTDepositPool contract allows users to deposit any of the supported assets into the protocol in exchange for RSETH.
119: function depositAsset( 120: address asset, 121: uint256 depositAmount 122: ) 123: external 124: whenNotPaused 125: nonReentrant 126: onlySupportedAsset(asset) 127: { 128: // checks 129: if (depositAmount == 0) { 130: revert InvalidAmount(); 131: } 132: if (depositAmount > getAssetCurrentLimit(asset)) { 133: revert MaximumDepositLimitReached(); 134: } 135: 136: if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) { 137: revert TokenTransferFailed(); 138: } 139: 140: // interactions 141: uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount); 142: 143: emit AssetDeposit(asset, depositAmount, rsethAmountMinted); 144: }
The RSETH mint amount depends on different factors, including the current price of the deposit asset, and the RSETH price, which also factors prices and amounts from all of the supported assets:
095: function getRsETHAmountToMint( 096: address asset, 097: uint256 amount 098: ) 099: public 100: view 101: override 102: returns (uint256 rsethAmountToMint) 103: { 104: // setup oracle contract 105: address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE); 106: ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress); 107: 108: // calculate rseth amount to mint based on asset amount and asset exchange rate 109: rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice(); 110: }
As the user is depositing a single asset, any change over any of the dependent factors may alter the minted amount the user receives for their deposit. Depositors don't have any slippage control over how much RSETH is minted for their deposit.
Add a minOutput
parameter to depositAsset()
to allow depositors to control the minted amount.
function depositAsset( address asset, - uint256 depositAmount + uint256 depositAmount, + uint256 minOutput ) external whenNotPaused nonReentrant onlySupportedAsset(asset) { // checks if (depositAmount == 0) { revert InvalidAmount(); } if (depositAmount > getAssetCurrentLimit(asset)) { revert MaximumDepositLimitReached(); } if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) { revert TokenTransferFailed(); } // interactions uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount); + if (rsethAmountMinted < minOutput) { + revert InsufficientOutputAmount(); + } emit AssetDeposit(asset, depositAmount, rsethAmountMinted); }
Other
#0 - c4-pre-sort
2023-11-16T23:35:29Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-11-16T23:35:37Z
raymondfam marked the issue as duplicate of #39
#2 - c4-pre-sort
2023-11-17T06:43:19Z
raymondfam marked the issue as duplicate of #148
#3 - c4-judge
2023-11-29T19:11:08Z
fatherGoose1 marked the issue as satisfactory
🌟 Selected for report: m_Rassska
Also found by: 0x1337, 0xAadi, 0xHelium, 0xLeveler, 0xblackskull, 0xbrett8571, 0xepley, 0xffchain, 0xluckhu, 0xmystery, 0xrugpull_detector, 0xvj, ABAIKUNANBAEV, Aamir, AerialRaider, Amithuddar, Bauchibred, Bauer, CatsSecurity, Cryptor, Daniel526, Draiakoo, Eigenvectors, ElCid, GREY-HAWK-REACH, Inspecktor, Juntao, King_, LinKenji, Madalad, MaslarovK, Matin, MatricksDeCoder, McToady, Noro, PENGUN, Pechenite, Phantasmagoria, RaoulSchaffranek, SBSecurity, SandNallani, Shaheen, Soul22, Stormreckson, T1MOH, Tadev, TeamSS, TheSchnilch, Topmark, Tumelo_Crypto, Udsen, Yanchuan, ZanyBonzy, _thanos1, adeolu, adriro, alexfilippov314, almurhasan, amaechieth, anarcheuz, ayden, baice, bareli, boredpukar, bronze_pickaxe, btk, cartlex_, catellatech, chaduke, cheatc0d3, circlelooper, codynhat, crack-the-kelp, critical-or-high, debo, deepkin, desaperh, dipp, eeshenggoh, evmboi32, ge6a, gesha17, glcanvas, gumgumzum, hals, hihen, hunter_w3b, jasonxiale, joaovwfreire, ke1caM, leegh, lsaudit, marchev, merlinboii, niser93, osmanozdemir1, paritomarrr, passion, pep7siup, phoenixV110, pipidu83, poneta, ro1sharkm, rouhsamad, rvierdiiev, sakshamguruji, seerether, shealtielanz, soliditytaker, spark, squeaky_cactus, stackachu, supersizer0x, tallo, taner2344, turvy_fuzz, twcctop, ubl4nk, wisdomn_, xAriextz, zach, zhaojie, zhaojohnson, ziyou-
2.7592 USDC - $2.76
Total of 7 issues:
ID | Issue |
---|---|
L-1 | Supported assets cannot be removed |
L-2 | Duplicate access list in RSETH contract |
L-3 | Admin could get locked out when updating LRTConfig |
L-4 | Missing storage gap in LRTConfigRoleChecker |
L-5 | No decimal normalization in price feeds |
L-6 | Potential denial of service due to unbounded gas usage in getTotalAssetDeposits() |
L-7 | Potential overflow in getAssetCurrentLimit() |
Total of 2 issues:
ID | Issue |
---|---|
NC-1 | Add an initializer to the LRTConfigRoleChecker contract |
NC-2 | Use UUPS over TransparentProxy |
Configured assets in the LRTConfig contract can be added by calling addNewSupportedAsset()
, but there is no current way of removing these assets if it is needed later.
73: function addNewSupportedAsset(address asset, uint256 depositLimit) external onlyRole(LRTConstants.MANAGER) { 74: _addNewSupportedAsset(asset, depositLimit); 75: }
Consider adding a function to let admins or managers to eventually remove an asset from the supported list.
The LRTConfig acts as an access list through the protocol, contracts inherit from LRTConfigRoleChecker that provides a link to the LRTConfig contract to check for access control.
However, the RSETH contract inherits from AccessControlUpgradeable and also uses LRTConfig, meaning this contract has two different access lists. Both of these access lists are mixed within the contract: mint and burn access go through the access list present in the contract itself, while pause is controlled via the onlyLRTManager
modifier present in the LRTConfig access list.
This is confusing and error prone. Consider using just the access list defined by LRTConfig.
Access control within the protocol is provided by an access list implemented in the LRTConfig contract.
This access list is used throughout the protocol by the LRTConfigRoleChecker mixin, which holds the reference to the current LRTConfig contract.
The config instance present in the LRTConfigRoleChecker contract can be updated by an admin using the updateLRTConfig()
function:
47: function updateLRTConfig(address lrtConfigAddr) external virtual onlyLRTAdmin { 48: UtilLib.checkNonZeroAddress(lrtConfigAddr); 49: lrtConfig = ILRTConfig(lrtConfigAddr); 50: emit UpdatedLRTConfig(lrtConfigAddr); 51: }
Once the new config takes place, the new access list from this instance will be effective. This means that if access control is not properly set up in the new instance, the admin could be locked out of the protocol.
Consider either:
updateLRTConfig()
.The LRTConfigRoleChecker contract is used as a base contract for upgradeable contracts but doesn't contain any storage gap to ensure enough space for future revisions.
abstract contract LRTConfigRoleChecker { ILRTConfig public lrtConfig; + uint256[50] private __gap;
Chainlink feeds simply returns the price without checking for any decimal discrepancy.
37: function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) { 38: return AggregatorInterface(assetPriceFeed[asset]).latestAnswer(); 39: }
In the case price feeds used by supported assets don't match in their decimals, the error will be carried forward during the calculation of the RSETH price in getRSETHPrice()
, as numbers with different precision will be aggregated together.
Currently, feeds for all supported assets (stETH, cbETH and rETH) have 18 decimals, but caution must be taken if other assets are added.
getTotalAssetDeposits()
The implementation of getTotalAssetDeposits()
is composed of the asset balance in the DepositPool, and the aggregation of all balances and deposits in EigenLayer for each of the registered node delegators.
47: function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) { 48: (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) = 49: getAssetDistributionData(asset); 50: return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer); 51: }
71: function getAssetDistributionData(address asset) 72: public 73: view 74: override 75: onlySupportedAsset(asset) 76: returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) 77: { 78: // Question: is here the right place to have this? Could it be in LRTConfig? 79: assetLyingInDepositPool = IERC20(asset).balanceOf(address(this)); 80: 81: uint256 ndcsCount = nodeDelegatorQueue.length; 82: for (uint256 i; i < ndcsCount;) { 83: assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]); 84: assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset); 85: unchecked { 86: ++i; 87: } 88: } 89: }
As node delegators are expected to grow over time, this may eventually cause a denial of service due to the unbounded gas usage required to compute the function.
Note that getTotalAssetDeposits()
is a key function in the protocol, as it is used in getRSETHPrice()
to calculate the RSETH price and in getAssetCurrentLimit()
, which is then used to check for current deposit limits in depositAsset()
.
A potential solution could be to track balances internally instead of aggregating all volume on the fly for each node delegator.
getAssetCurrentLimit()
The implementation of getAssetCurrentLimit()
subtracts the configured limit for the asset to the current total deposits in the protocol:
56: function getAssetCurrentLimit(address asset) public view override returns (uint256) { 57: return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset); 58: }
As deposits held in EigenLayer are expected to grow over time, it is technically possible for getTotalAssetDeposits(asset)
to be greater than lrtConfig.depositLimitByAsset(asset)
, causing an overflow.
Consider returning 0
if this is this happens:
function getAssetCurrentLimit(address asset) public view override returns (uint256) { uint256 limit = lrtConfig.depositLimitByAsset(asset); uint256 balance = getTotalAssetDeposits(asset); if (limit > balance) { unchecked { return limit - balance; } } return 0; }
All the contracts that inherit from LRTConfigRoleChecker have to initialize the lrtConfig
variable in its own initializer, having this code duplicated across all contracts.
Add an initializer to LRTConfigRoleChecker to have the derived contract use it during initialization.
function __LRTConfigRoleChecker_init(address lrtConfigAddr) internal onlyInitializing { __LRTConfigRoleChecker_init_unchained(lrtConfigAddr); } function __LRTConfigRoleChecker_init_unchained(address lrtConfigAddr) internal onlyInitializing { lrtConfig = ILRTConfig(lrtConfigAddr); emit UpdatedLRTConfig(lrtConfigAddr); }
The UUPS pattern is more flexible than the TransparentProxy as the upgrade logic resides in the implementation itself. It is also cheaper to use, since the Proxy doesn't need to check if the caller is the admin to switch between logics.
The general recommendation is to use UUPS over TransparentProxy, see Transparent vs UUPS Proxies.
#0 - c4-pre-sort
2023-11-17T23:16:44Z
raymondfam marked the issue as sufficient quality report
#1 - raymondfam
2023-11-17T23:18:50Z
Possible upgrade:
L-5 --> #479
#2 - c4-judge
2023-12-01T16:47:36Z
fatherGoose1 marked the issue as grade-b
#3 - c4-judge
2023-12-01T18:03:24Z
fatherGoose1 removed the grade
#4 - c4-judge
2023-12-01T18:43:56Z
fatherGoose1 marked the issue as grade-b