This repository has been archived by the owner on Oct 20, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 3
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
a0cb989
commit 2094662
Showing
176 changed files
with
10,372 additions
and
10 deletions.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
Brisk Mercurial Dinosaur | ||
|
||
high | ||
|
||
# Vulnerability in EthAssetManager Contract's exchangeRate() Function Allows for Manipulation of Exchange Rate Calculation | ||
|
||
### Summary | ||
|
||
The EthAssetManager contract has a vulnerability in the `exchangeRate()` function that can allow an attacker to manipulate the exchange rate calculation by providing malicious input to the `metaPool.get_dy()` function. | ||
|
||
### Vulnerability Detail | ||
|
||
The `exchangeRate()` function in the EthAssetManager contract calculates the exchange rate between the ALETH and ETH assets in the meta pool. The function uses the `metaPool.get_dy()` function to calculate the change in the ETH asset balance for a given change in the ALETH asset balance. However, the `metaPool.get_dy()` function is not checked for errors, and an attacker can potentially provide malicious input to this function, causing it to return an unexpected value and leading to an incorrect exchange rate calculation. | ||
|
||
### Impact | ||
|
||
If an attacker can manipulate the exchange rate calculation, they can potentially perform arbitrage attacks on the meta pool, causing losses to the pool and its users. | ||
|
||
### Code Snippet | ||
https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/EthAssetManager.sol#L253-L264 | ||
The vulnerable code in the `exchangeRate()` function is shown below: | ||
```scss | ||
function exchangeRate() public view returns (uint256) { | ||
IERC20 alETH = getTokenForMetaPoolAsset(MetaPoolAsset.ALETH); | ||
|
||
uint256[NUM_META_COINS] memory metaBalances = metaPool.get_balances(); | ||
return metaPool.get_dy( | ||
int128(uint128(uint256(MetaPoolAsset.ALETH))), | ||
int128(uint128(uint256(MetaPoolAsset.ETH))), | ||
10**SafeERC20.expectDecimals(address(alETH)), | ||
metaBalances | ||
); | ||
} | ||
``` | ||
### Tool used | ||
|
||
Manual Review | ||
|
||
### Recommendation | ||
|
||
To address this vulnerability, the EthAssetManager contract should implement proper error handling and validation of the return values from the `metaPool.get_dy()` function. For example, the contract could use a try-catch block to handle any potential errors and take appropriate action, such as reverting the transaction or logging the error. Additionally, the contract could validate the input parameters to the `metaPool.get_dy()` function to ensure that they are within expected ranges and cannot be manipulated by an attacker. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
Passive Mahogany Starling | ||
|
||
medium | ||
|
||
# No check for active L2 Sequencer | ||
|
||
## Summary | ||
Chainlink recommends that all Optimistic L2 oracles consult the Sequencer Uptime Feed to ensure that the sequencer is live before trusting the data returned by the oracle. There is no check in the OptimismRewardCollector protocol. | ||
|
||
## Vulnerability Detail | ||
According to the documentation, only Gelato and Connext are trusted, while the sequencer (external admin) is not trusted by the protocol. | ||
```solidity | ||
Gelato - Gelato is trusted. Gelato can be unresponsive - if gelato is unresponsive, the harvest will fail. This is acceptable as Alchemist admins can manually trigger harvests, and users will still accumulate credit/yield while waiting for a harvest. | ||
Connext - Connext is trusted to operate properly, be responsive, and not steal funds or mint unbacked assets. | ||
``` | ||
If the Sequencer goes down, oracle data will not be kept up to date, and thus could become stale. However, users are able to continue to interact with the protocol directly through the L1 optimistic rollup contract. You can review Chainlink docs on [L2 Sequencer Uptime Feeds](https://docs.chain.link/docs/data-feeds/l2-sequencer-feeds/) for more details on this. | ||
|
||
As a result, users may be able to use the protocol while oracle feeds are stale. This could cause many problems. | ||
|
||
## Impact | ||
If the sequencer goes down, the protocol will use outdated prices. | ||
|
||
|
||
## Code Snippet | ||
https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L78-L129 | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
It is recommended to follow the code example of Chainlink: https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
Passive Mahogany Starling | ||
|
||
high | ||
|
||
# The calculated value for slippage protection in the protocol is inaccurate | ||
|
||
## Summary | ||
|
||
|
||
The protocol calculates the slippage protection value based on the price of OP relative to USD and OP relative to ETH, while the intended exchange is for alUSD and alETH. This results in inaccuracies in the calculated slippage protection value. | ||
|
||
## Vulnerability Detail | ||
|
||
In the `RewardRouter.distributeRewards()` function, the protocol first sends the OP rewards to the OptimismRewardCollector contract, | ||
```solidity | ||
TokenUtils.safeTransfer(IRewardCollector(rewards[vault].rewardCollectorAddress).rewardToken(), rewards[vault].rewardCollectorAddress, amountToSend); | ||
rewards[vault].lastRewardBlock = block.number; | ||
rewards[vault].rewardPaid += amountToSend; | ||
``` | ||
|
||
|
||
then calls the `RewardCollector.claimAndDonateRewards()` function to convert OP into alUSD or alETH. | ||
```solidity | ||
return IRewardCollector(rewards[vault].rewardCollectorAddress).claimAndDonateRewards(vault, IRewardCollector(rewards[vault].rewardCollectorAddress).getExpectedExchange(vault) * slippageBPS / BPS); | ||
``` | ||
|
||
During the conversion process, there is a parameter for slippage protection, which is calculated using `OptimismRewardCollector.getExpectedExchange() * slippageBPS / BPS`. Let's take a look at the `getExpectedExchange()` function. In this function, the protocol retrieves the prices of optoUSD and optoETH from Chainlink. | ||
|
||
```solidity | ||
( | ||
uint80 roundID, | ||
int256 opToUsd, | ||
, | ||
uint256 updateTime, | ||
uint80 answeredInRound | ||
) = IChainlinkOracle(opToUsdOracle).latestRoundData(); | ||
``` | ||
```solidity | ||
// Ensure that round is complete, otherwise price is stale. | ||
( | ||
uint80 roundIDEth, | ||
int256 ethToUsd, | ||
, | ||
uint256 updateTimeEth, | ||
uint80 answeredInRoundEth | ||
) = IChainlinkOracle(ethToUsdOracle).latestRoundData(); | ||
``` | ||
|
||
If `debtToken == alUsdOptimism`, the expectedExchange for slippage protection is calculated as totalToSwap * uint(opToUsd) / 1e8. | ||
```solidity | ||
// Find expected amount out before calling harvest | ||
if (debtToken == alUsdOptimism) { | ||
expectedExchange = totalToSwap * uint(opToUsd) / 1e8; | ||
``` | ||
|
||
If debtToken == alEthOptimism, the expectedExchange for slippage protection is calculated as totalToSwap * uint(uint(opToUsd)) / uint(ethToUsd). | ||
```solidity | ||
else if (debtToken == alEthOptimism) { | ||
expectedExchange = totalToSwap * uint(uint(opToUsd)) / uint(ethToUsd); | ||
``` | ||
|
||
Here, we observe that the `expectedExchange` is calculated based on the value of OP relative to USD and OP relative to ETH, while the protocol intends to exchange for alUSD and alETH. | ||
```solidity | ||
if (debtToken == 0xCB8FA9a76b8e203D8C3797bF438d8FB81Ea3326A) { | ||
// Velodrome Swap Routes: OP -> USDC -> alUSD | ||
IVelodromeSwapRouter.route[] memory routes = new IVelodromeSwapRouter.route[](2); | ||
routes[0] = IVelodromeSwapRouter.route(0x4200000000000000000000000000000000000042, 0x7F5c764cBc14f9669B88837ca1490cCa17c31607, false); | ||
routes[1] = IVelodromeSwapRouter.route(0x7F5c764cBc14f9669B88837ca1490cCa17c31607, 0xCB8FA9a76b8e203D8C3797bF438d8FB81Ea3326A, true); | ||
TokenUtils.safeApprove(rewardToken, swapRouter, amountRewardToken); | ||
IVelodromeSwapRouter(swapRouter).swapExactTokensForTokens(amountRewardToken, minimumAmountOut, routes, address(this), block.timestamp); | ||
} else if (debtToken == 0x3E29D3A9316dAB217754d13b28646B76607c5f04) { | ||
// Velodrome Swap Routes: OP -> alETH | ||
IVelodromeSwapRouter.route[] memory routes = new IVelodromeSwapRouter.route[](1); | ||
routes[0] = IVelodromeSwapRouter.route(0x4200000000000000000000000000000000000042, 0x3E29D3A9316dAB217754d13b28646B76607c5f04, false); | ||
TokenUtils.safeApprove(rewardToken, swapRouter, amountRewardToken); | ||
IVelodromeSwapRouter(swapRouter).swapExactTokensForTokens(amountRewardToken, minimumAmountOut, routes, address(this), block.timestamp); | ||
} | ||
``` | ||
|
||
However, the price of alUSD is not equivalent to USD, and the price of alETH is not equivalent to ETH. This discrepancy leads to inaccuracies in the calculated value for slippage protection, making the protocol vulnerable to sandwich attacks. | ||
|
||
## Impact | ||
The protocol is susceptible to sandwich attacks. | ||
|
||
|
||
## Code Snippet | ||
https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L120-L126 | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
Calculate using the correct prices. | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
Muscular Ivory Elk | ||
|
||
medium | ||
|
||
# Price can be stale | ||
|
||
## Summary | ||
|
||
there are not enough checks to ensure that the return price is valid. | ||
|
||
## Vulnerability Detail | ||
|
||
getExpectedExchange() in OptimismRewardCollector.sol uses latestRoundData to get the latest price. However, there is no check for if the return value is stale data. | ||
|
||
## Impact | ||
|
||
The staleness of the chainlink return values will lead to wrong calculation | ||
|
||
## Code Snippet | ||
|
||
https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L78-L128 | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
|
||
``` | ||
require(answeredInRound >= roundId, "price is stale"); | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
Muscular Ivory Elk | ||
|
||
medium | ||
|
||
# Price can be stale | ||
|
||
## Summary | ||
|
||
there are not enough checks to ensure that the return price is valid. | ||
|
||
## Vulnerability Detail | ||
|
||
getExpectedExchange() in OptimismRewardCollector.sol uses latestRoundData to get the latest price. However, there is no check for if the return value is stale data. | ||
|
||
## Impact | ||
|
||
The staleness of the chainlink return values will lead to wrong calculation | ||
|
||
## Code Snippet | ||
|
||
[2024-04-alchemix-mitkolyungov/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol](https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L78-L128) | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
```solidity | ||
require(answeredInRound >= roundId, "price is stale"); | ||
``` | ||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
Slow Brown Coyote | ||
|
||
high | ||
|
||
# No check for Arbitrum/Optimisim sequencer down in Chainlink feeds | ||
|
||
## Summary | ||
When utilizing Chainlink in L2 chains like Arbitrum, it's important to ensure that the prices provided are not falsely perceived as fresh, even when the sequencer is down. This vulnerability could potentially be exploited by malicious actors to gain an unfair advantage. | ||
|
||
## Vulnerability Detail | ||
If it updates again within the update threshold. The feeds typically can update several times within a threshold period if the price is moving a lot when the sequencer is down, the new price won't be reported to the chain. the feed on the L2 will return the value it had when it went down | ||
```solidity | ||
function getExpectedExchange(address yieldToken) external view returns (uint256) { | ||
uint256 expectedExchange; | ||
address[] memory token = new address[](1); | ||
uint256 totalToSwap = TokenUtils.safeBalanceOf(rewardToken, address(this)); | ||
// Ensure that round is complete, otherwise price is stale. | ||
( | ||
uint80 roundID, | ||
int256 opToUsd, | ||
, | ||
uint256 updateTime, | ||
uint80 answeredInRound | ||
) = IChainlinkOracle(opToUsdOracle).latestRoundData(); | ||
require( | ||
opToUsd > 0, | ||
"Chainlink Malfunction" | ||
); | ||
if( updateTime < block.timestamp - 1200 seconds ) { | ||
revert("Chainlink Malfunction"); | ||
} | ||
// Ensure that round is complete, otherwise price is stale. | ||
( | ||
uint80 roundIDEth, | ||
int256 ethToUsd, | ||
, | ||
uint256 updateTimeEth, | ||
uint80 answeredInRoundEth | ||
) = IChainlinkOracle(ethToUsdOracle).latestRoundData(); | ||
require( | ||
ethToUsd > 0, | ||
"Chainlink Malfunction" | ||
); | ||
if( updateTimeEth < block.timestamp - 1200 seconds ) { | ||
revert("Chainlink Malfunction"); | ||
} | ||
``` | ||
This causes the `RewardRouter::distributeRewards` to donate undervalued value of rewards. | ||
## Impact | ||
Potentially be exploited by malicious actors to gain an unfair advantage,k causing protocol to donate the undervalued rewards. | ||
## Code Snippet | ||
https://github.com/sherlock-audit/2024-04-alchemix/blob/32e4902f77b05ea856cf52617c55c3450507281c/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L97C1-L130C10 | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
code example of Chainlink: https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code |
Oops, something went wrong.