Closed sherlock-admin closed 1 year ago
Escalate for 10 USDC Hi, please review this Issue again, I think the problem exists, the principle is described above. For a clearer demonstration, I wrote a POC:
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
contract InvariantTest is Test {
Loop internal loop;
function setUp() public {
loop = new Loop();
targetContract(address(loop));
bytes4[] memory selectors = new bytes4[](1);
selectors[0] = loop.breakLoop.selector;
targetSelector(FuzzSelector({
addr: address(loop),
selectors: selectors
}));
}
function invariant_loopDOS() public view {
assert(loop.v() == false);
}
}
contract Loop {
uint256 internal constant SIZE = 8192;
bool public v;
function testLoopDOS() public {
loopDOS(8192);
}
function loopDOS(uint256 sumIndex_) internal {
++sumIndex_;
uint256 j = SIZE; // bit that iterates from MSB to LSB
uint256 index = 0; // build up sumIndex bit by bit
// Used to terminate loop. We don't need to consider final 0 bits of sumIndex_
uint256 indexLSB = lsb(sumIndex_);
uint256 curIndex;
while (j >= indexLSB) {
curIndex = index + j;
// Skip considering indices outside bounds of Fenwick tree
if (curIndex > SIZE) continue;
if (sumIndex_ & j != 0) {
// Build up index bit by bit
index = curIndex;
// terminate if we've already matched sumIndex_
if (index == sumIndex_) break;
}
// shift j to consider next less signficant bit
j = j >> 1;
}
}
function breakLoop(uint256 sumIndex_) external {
require(sumIndex_ <= SIZE);
++sumIndex_;
uint256 j = SIZE; // bit that iterates from MSB to LSB
uint256 index = 0; // build up sumIndex bit by bit
// Used to terminate loop. We don't need to consider final 0 bits of sumIndex_
uint256 indexLSB = lsb(sumIndex_);
uint256 curIndex;
while (j >= indexLSB) {
curIndex = index + j;
// Skip considering indices outside bounds of Fenwick tree
if (curIndex > SIZE) {
v = true;
break;
}
if (sumIndex_ & j != 0) {
// Build up index bit by bit
index = curIndex;
// terminate if we've already matched sumIndex_
if (index == sumIndex_) break;
}
// shift j to consider next less signficant bit
j = j >> 1;
}
}
function lsb(
uint256 i_
) internal pure returns (uint256 lsb_) {
if (i_ != 0) {
// "i & (-i)"
lsb_ = i_ & ((i_ ^ 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) + 1);
}
}
}
Run the following command and you will find that the program will be DOS:
forge test --match-test testLoopDOS -vvvv
Escalate for 10 USDC Hi, please review this Issue again, I think the problem exists, the principle is described above. For a clearer demonstration, I wrote a POC:
// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13; import "forge-std/Test.sol"; contract InvariantTest is Test { Loop internal loop; function setUp() public { loop = new Loop(); targetContract(address(loop)); bytes4[] memory selectors = new bytes4[](1); selectors[0] = loop.breakLoop.selector; targetSelector(FuzzSelector({ addr: address(loop), selectors: selectors })); } function invariant_loopDOS() public view { assert(loop.v() == false); } } contract Loop { uint256 internal constant SIZE = 8192; bool public v; function testLoopDOS() public { loopDOS(8192); } function loopDOS(uint256 sumIndex_) internal { ++sumIndex_; uint256 j = SIZE; // bit that iterates from MSB to LSB uint256 index = 0; // build up sumIndex bit by bit // Used to terminate loop. We don't need to consider final 0 bits of sumIndex_ uint256 indexLSB = lsb(sumIndex_); uint256 curIndex; while (j >= indexLSB) { curIndex = index + j; // Skip considering indices outside bounds of Fenwick tree if (curIndex > SIZE) continue; if (sumIndex_ & j != 0) { // Build up index bit by bit index = curIndex; // terminate if we've already matched sumIndex_ if (index == sumIndex_) break; } // shift j to consider next less signficant bit j = j >> 1; } } function breakLoop(uint256 sumIndex_) external { require(sumIndex_ <= SIZE); ++sumIndex_; uint256 j = SIZE; // bit that iterates from MSB to LSB uint256 index = 0; // build up sumIndex bit by bit // Used to terminate loop. We don't need to consider final 0 bits of sumIndex_ uint256 indexLSB = lsb(sumIndex_); uint256 curIndex; while (j >= indexLSB) { curIndex = index + j; // Skip considering indices outside bounds of Fenwick tree if (curIndex > SIZE) { v = true; break; } if (sumIndex_ & j != 0) { // Build up index bit by bit index = curIndex; // terminate if we've already matched sumIndex_ if (index == sumIndex_) break; } // shift j to consider next less signficant bit j = j >> 1; } } function lsb( uint256 i_ ) internal pure returns (uint256 lsb_) { if (i_ != 0) { // "i & (-i)" lsb_ = i_ & ((i_ ^ 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) + 1); } } }
Run the following command and you will find that the program will be DOS:
forge test --match-test testLoopDOS -vvvv
You've created a valid escalation for 10 USDC!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Thanks for the PoC. I am unsure if that state can ever arrive because uint256 internal constant SIZE = 8192;
so for if (curIndex > SIZE) continue;
to reach seems impossible. Let me know otherwise @grandizzy
It appears that there is indeed a bug in the library in that if it were called with an index beyond the range of the deposits, it would go into an infinite loop. The code should have a break
and not a continue
here. We would dispute the severity as low/informative
because the library is never invoked in that range of inputs however.
Result: Low Unique Considering the impact of this valid low based on sponsor comment.
kutugu
medium
Deposits prefixSum function can go into a DOS loop
Summary
Deposits prefixSum function use a while loop, there is a continue branch in the while, and entering this branch will cause an endless loop, result in out of gas and tx revert.
Vulnerability Detail
If
j >= indexLSB
andcurIndex > SIZE
, the continue statement is always fired and the state machine does not change, resulting in an endless loop.Impact
accrueInterest
,updateInterestState
depend on theprefixSum
, so this will affect the entire protocol.Code Snippet
Tool used
Manual Review
Recommendation
This is supposed to be a
j++
state change