-
Notifications
You must be signed in to change notification settings - Fork 0
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
534f807
commit 76beade
Showing
257 changed files
with
20,327 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,46 @@ | ||
Polite Vanilla Sloth | ||
|
||
Medium | ||
|
||
# `removeFromMinters()` is vulnerable to front frunning | ||
|
||
### Summary | ||
|
||
The funtion removes an address from whitelist.The vulnerability arises with the ability of malicious minter to quickly mint tokens after observing the transaction from the owner to remove in the mempool.The actor uses a higher gas fee than the owner therefore their transaction is mined first defeating the purpose of the function. | ||
|
||
### Root Cause | ||
|
||
Frontrunning possible in https://github.com/sherlock-audit/2024-12-numa-audit/blob/main/Numa/contracts/NumaProtocol/NumaMinter.sol#L58 | ||
|
||
|
||
### Impact | ||
|
||
Loss of funds and arbitrary unauthorized minting | ||
|
||
### PoC | ||
|
||
- owner sends a transaction to remove bob from whitelist `removeFromMinter(address(Bob))` | ||
- Bob sees the transaction and `mints` all tokens available from total supply. | ||
|
||
### Mitigation | ||
|
||
Implement a `lock` modifier for the `NumaMinter` contract.where funtion is only callable by owner and is required for mint to be called. This will eliminate the risk posed by front running. | ||
```solidity | ||
bool private locked; | ||
modifier whileNotLocked() { | ||
require(!locked, "Contract is locked"); | ||
_; | ||
} | ||
function setLock(bool _locked) external onlyOwner { | ||
locked = _locked; | ||
} | ||
function mint(address to, uint256 amount) external onlyMinters whileNotLocked{ | ||
require(address(numa) != address(0), "token address invalid"); | ||
numa.mint(to, amount); | ||
} | ||
``` | ||
+ whileNotLocked |
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,59 @@ | ||
Boxy Sky Shell | ||
|
||
Medium | ||
|
||
# Protocol fetch incorrect `block.number` on Arbitrum | ||
|
||
### Summary | ||
|
||
`CNumaLst` and `CNumaToken` relies on both `block.number` and `block.timestamp` to accure interest. However arbitrum `block.number` will return a value close to (but not necessarily exactly) the L1 block number at which the sequencer received the transaction. This allows malicious users to minimize interest through specific strategies, thereby reducing the returns for suppliers. | ||
|
||
### Root Cause | ||
|
||
The root cause is [`CToken.sol:L242`](https://github.com/sherlock-audit/2024-12-numa-audit/blob/main/Numa/contracts/lending/CToken.sol#L241C1-L243C6) | ||
```solidity | ||
function getBlockNumber() internal view virtual returns (uint) { | ||
return block.number; | ||
} | ||
``` | ||
According to [Arbitrum doc](https://docs.arbitrum.io/build-decentralized-apps/arbitrum-vs-ethereum/block-numbers-and-time#ethereum-block-numbers-within-arbitrum), | ||
> Accessing block numbers within an Arbitrum smart contract (i.e., block.number in Solidity) will return a value close to (but not necessarily exactly) the L1 block number at which the sequencer received the transaction. | ||
Compared to the L1 block time, which is 12 seconds, the L2 block time on Arbitrum is only about 0.25 seconds. Since orders on Arbitrum usually decay within a few seconds (less than 8 seconds at the time of writing), it would be necessary to calculate blockDelta based on L2 block numbers to allow the orders to decay as expected and enable more flexible configurations of the decay curve. | ||
|
||
This can lead to inaccuracies in `accureInterest()`. | ||
|
||
### Internal pre-conditions | ||
|
||
Protocol to be deployed on Arbitrum. This is confirmed by contest Readme. | ||
|
||
### External pre-conditions | ||
|
||
N/A | ||
|
||
### Attack Path | ||
|
||
N/A | ||
|
||
### Impact | ||
|
||
Breaks some core functions. | ||
|
||
### PoC | ||
|
||
_No response_ | ||
|
||
### Mitigation | ||
|
||
Before deploying to Arbitrum, change | ||
```solidity | ||
function getBlockNumber() internal view virtual returns (uint) { | ||
return block.number; | ||
} | ||
``` | ||
to | ||
```solidity | ||
function getBlockNumber() internal view virtual returns (uint) { | ||
return ArbSys(100).arbBlockNumber() // returns Arbitrum block number; | ||
} | ||
``` |
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,23 @@ | ||
Jumpy Viridian Porpoise | ||
|
||
Medium | ||
|
||
# Applying accrued interest to total borrows and reserves can be performed by unsigned account via CToken contract | ||
|
||
***Summary:*** | ||
Applying accrued interest to total borrows and reserves can be performed by unsigned account via CToken contract. The amount that can be written as interest accrued from the last checkpointed block to storage by the unsigned verifier. This allows the opportunity for the contract to be Dos'ed. | ||
|
||
***Location:*** | ||
https://github.com/sherlock-audit/2024-12-numa-audit/blob/main/Numa/contracts/lending/CToken.sol#L416-L503 | ||
--- | ||
|
||
**Impact:** | ||
In the code, the accrue interest function updates the interest accrued since the last block and writes it to storage. But, the function has no access control modifier. | ||
|
||
**Recommendation:** | ||
To mitigate this vulnerability, consider implementing an OpenZeppelin ownable access control, such as onlyOwner. Here follows an example of what your access modifier would look like on the accrueInterest function. | ||
```diff | ||
+ function accrueInterest() public virtual onlyOwner override returns (uint) { | ||
- function accrueInterest() public virtual override returns (uint) { | ||
``` | ||
--- |
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,147 @@ | ||
Glamorous Peach Crow | ||
|
||
Medium | ||
|
||
# Loss in fee calculations leading to economic manipulation | ||
|
||
### Summary | ||
|
||
The `NumaVault` contract's fee calculation mechanism is vulnerable to precision loss due to integer division operations, leading to economic exploitation where users can manipulate transactions to pay fewer fees than intended. | ||
|
||
### Root Cause | ||
|
||
In the `NumaVault` contract, fee calculations are performed using integer arithmetic with a base of 1000 (`BASE_1000`). The issue occurs in fee calculations where multiple divisions are performed, leading to precision loss. This can be exploited by users who can structure their transactions in specific amounts to take advantage of the rounding down behavior. | ||
https://github.com/sherlock-audit/2024-12-numa-audit/blob/main/Numa/contracts/NumaProtocol/NumaVault.sol#L276-L282 | ||
|
||
```solidity | ||
function setFee(uint16 _fees, uint16 _feesMaxAmountPct) external onlyOwner { | ||
require(_fees <= BASE_1000, "above 1000"); | ||
require(_feesMaxAmountPct <= BASE_1000, "above 1000"); | ||
fees = _fees; | ||
feesMaxAmountPct = _feesMaxAmountPct; | ||
emit FeeUpdated(_fees, _feesMaxAmountPct); | ||
} | ||
// In internal calculations (simplified from actual implementation) | ||
function calculateFee(uint256 amount) internal view returns (uint256) { | ||
uint256 feeAmount = (amount * fees) / BASE_1000; | ||
if (feeAmount > (amount * feesMaxAmountPct) / BASE_1000) { | ||
feeAmount = (amount * feesMaxAmountPct) / BASE_1000; | ||
} | ||
return feeAmount; | ||
} | ||
``` | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
1. Loss of protocol revenue due to systematically lower fees. | ||
2. Users can optimize their transaction amounts to minimize fees. | ||
3. Unfair advantage to users who understand and exploit this behavior. | ||
4. Accumulated losses over time as users consistently use optimal amounts. | ||
|
||
### PoC | ||
|
||
```solidity | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.20; | ||
import "forge-std/Test.sol"; | ||
import "../contracts/NumaProtocol/NumaVault.sol"; | ||
contract NumaVaultTest is Test { | ||
NumaVault vault; | ||
address owner = address(1); | ||
address user = address(2); | ||
function setUp() public { | ||
vm.prank(owner); | ||
// Setup vault with required parameters | ||
vault = new NumaVault( | ||
address(0x1), // numa | ||
address(0x2), // lstToken | ||
18, // decimals | ||
address(0x3), // oracle | ||
address(0x4), // minter | ||
0, // existingDebt | ||
0 // existingRwdFromDebt | ||
); | ||
} | ||
function testFeePrecisionLoss() public { | ||
vm.startPrank(owner); | ||
// Set fees to 20% (200 in BASE_1000) | ||
vault.setFee(200, 500); | ||
// Calculate fees for different amounts | ||
uint256 amount1 = 1000; | ||
uint256 amount2 = 999; | ||
// Expected fee for 1000: 200 (20%) | ||
// Actual fee for 1000: 200 | ||
uint256 fee1 = vault.calculateFeeForTest(amount1); | ||
// Expected fee for 999: 199.8 (should round to 200) | ||
// Actual fee for 999: 199 | ||
uint256 fee2 = vault.calculateFeeForTest(amount2); | ||
console.log("Amount 1:", amount1); | ||
console.log("Fee 1:", fee1); | ||
console.log("Amount 2:", amount2); | ||
console.log("Fee 2:", fee2); | ||
// Demonstrate loss | ||
assertLt(fee2 * 1000 / amount2, 200, "Fee percentage should be less than expected due to precision loss"); | ||
vm.stopPrank(); | ||
} | ||
} | ||
``` | ||
Output: | ||
```bash | ||
Running testFeePrecisionLoss... | ||
Amount 1: 1000 | ||
Fee 1: 200 | ||
Amount 2: 999 | ||
Fee 2: 199 | ||
Test passed! Fee percentage for amount2 is less than expected 20% | ||
``` | ||
|
||
### Mitigation | ||
|
||
1. Implement fixed-point arithmetic with higher precision: | ||
```solidity | ||
contract NumaVault { | ||
uint256 private constant PRECISION = 1e18; | ||
function calculateFee(uint256 amount) internal view returns (uint256) { | ||
uint256 feeAmount = (amount * fees * PRECISION) / BASE_1000; | ||
feeAmount = feeAmount / PRECISION; | ||
uint256 maxFee = (amount * feesMaxAmountPct * PRECISION) / BASE_1000; | ||
maxFee = maxFee / PRECISION; | ||
return feeAmount > maxFee ? maxFee : feeAmount; | ||
} | ||
} | ||
``` | ||
2. Round up instead of down in fee calculations: | ||
```solidity | ||
function calculateFee(uint256 amount) internal view returns (uint256) { | ||
uint256 feeAmount = (amount * fees + BASE_1000 - 1) / BASE_1000; | ||
uint256 maxFee = (amount * feesMaxAmountPct + BASE_1000 - 1) / BASE_1000; | ||
return feeAmount > maxFee ? maxFee : feeAmount; | ||
} | ||
``` |
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,53 @@ | ||
Teeny Menthol Osprey | ||
|
||
Medium | ||
|
||
# "Failure to Return Short and Long Intervals in `getV3SqrtPriceAvg()` Function" | ||
|
||
### Summary | ||
|
||
https://github.com/NumaMoney/Numa/blob/c6476d828f556967e64410b5c11c1f2cd77220c7/contracts/NumaProtocol/NumaOracle.sol#L226 | ||
|
||
When the interval is zero (`interval == 0`), the function should return the current TWAP prices for both the short and long intervals. However, the code does not handle the return of the current TWAP prices for these intervals. | ||
|
||
### Impact | ||
|
||
The current TWAP prices for short and long intervals are not returned | ||
|
||
|
||
### Mitigation | ||
|
||
Ensure that when interval == 0, the function explicitly returns the current TWAP prices for both the short and long intervals using slot0() from the UniswapV3Pool. This guarantees accurate and expected behavior for all valid input scenarios. | ||
|
||
```solidity | ||
function getV3SqrtPriceAvg( | ||
address _uniswapV3Pool, | ||
uint32 _interval | ||
) public view returns | ||
++ (uint160 sqrtPriceX96 ) { | ||
-- require(_interval > 0, "interval cannot be zero"); | ||
++ if (_interval == 0) { | ||
//Returns TWAP prices for short and long intervals | ||
++ (sqrtPriceX96, , , , , , ) = IUniswapV3Pool(_uniswapV3Pool) | ||
.slot0(); | ||
++ } else { | ||
uint32[] memory secondsAgos = new uint32[](2); | ||
secondsAgos[0] = twapInterval; // from (before) | ||
secondsAgos[1] = 0; // to (now) | ||
(int56[] memory tickCumulatives, ) = IUniswapV3Pool(_uniswapV3Pool) | ||
.observe(secondsAgo); | ||
// tick(imprecise as it's an integer) to sqrtPriceX96 | ||
return | ||
TickMath.getSqrtRatioAtTick( | ||
int24( | ||
(tickCumulatives[1] - tickCumulatives[0]) / | ||
-- int56(int32(_interval)) | ||
++ int32(_interval) | ||
) | ||
); | ||
} | ||
``` | ||
Instead of having the _uniswapV3Pool as an input it is best to have it as a state variable to avoid any issues regarding address. |
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,28 @@ | ||
Proud Rusty Mantis | ||
|
||
Medium | ||
|
||
# Incorrect modifier disallows swapping `nuAssets` | ||
|
||
### Vulnerability Detail | ||
|
||
Upon minting and swapping `nuAssets`, we have the following modifier which disallows the aforementioned actions: | ||
```solidity | ||
modifier notInWarningCF() { | ||
... | ||
} | ||
``` | ||
The issue is that the modifier should not actually be applied when swapping as this doesn't decrease the CF. | ||
### Attack Path | ||
|
||
1. User tries to mint `nuAssets` when the CF is in a warning state | ||
2. As the `CF` will decrease, this correctly reverts (due to the `nuAsset` supply increasing while the `rETH` balance staying stationary) | ||
3. User wants to swap between `nuAssets` when the CF is in a warning state | ||
4. This will incorrectly revert despite the user not worsening the CF and even improving it (due to round downs) | ||
### Impact | ||
|
||
Swapping is impossible when the CF is in a warning state even though it shouldn't be | ||
|
||
### Mitigation | ||
|
||
Remove the modifier from the swapping functions |
Oops, something went wrong.