search

sherlock-audit / 2023-02-bond-judging

2 stars 0 forks source link

fat32 - SWC-101 Arithmetic Underflow Underflow on Block Timestamp. BondChainlinkOracle.t.sol BondChainlinkOracle.sol function testFuzz_currentPrice_oneFeed() #7

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

fat32

medium

SWC-101 Arithmetic Underflow Underflow on Block Timestamp. BondChainlinkOracle.t.sol BondChainlinkOracle.sol function testFuzz_currentPrice_oneFeed()

SWC-101 Arithmetic Underflow on Block Timestamp. BondChainlinkOracle.t.sol BondChainlinkOracle.sol function testFuzz_currentPrice_oneFeed(uint8, uint8, uint48)

Summary

The test to set block timestamp past the update threshold should fail. But it does not when using integer underflow. 1 call out of 10 calls the test to set block timestamp past the update threshold passes and does not revert using integer underflow when it never should. The result is a bad price feed was registered.

Vulnerability Detail

// In the function
function testFuzz_currentPrice_oneFeed(uint8, uint8, uint48) 
// the following line was suffixed with integer underflow.  
// Set block timestamp past the update threshold, should fail
        vm.warp(block.timestamp + numerUpdateThreshold_ + 1 - 1e18);
        vm.expectRevert(err);
        oracle.currentPrice(0);

Impact

The test to set block timestamp past the update threshold should fail. But it does not when using integer underflow. This means that the block timestamp can be manipulated. 1 call out of 10 calls the test to set block timestamp past the update threshold passes and does not revert using integer underflow when it never should.
So, being able to set the block timestamp past the update threshold means that one can write to that blockchain. And in so doing, making them owner and being able to withdraw money or funds, which the log file dump below displays in summary. The result is a bad price feed was registered.

Code Snippet

Vulnerable code

https://github.com/sherlock-audit/2023-02-bond/blob/main/bonds/src/BondChainlinkOracle.sol#L129-L148

POC> 2023-02-bond-0xtr3/bonds/src/test/BondChainlinkOracle.t.sol

// Set block timestamp past the update threshold, should fail
        vm.warp(block.timestamp + numerUpdateThreshold_ + 1 - 1e18);  // fat32
        vm.expectRevert(err);
        oracle.currentPrice(0);
forge test -vvv --match-path src/test/BondChainlinkOracle.t.sol

Tool used

Foundry and Visual Studio Code.

Manual Review Log File:

forge test -vvv --match-path src/test/BondChainlinkOracle.t.sol
[⠆] Compiling...
No files changed, compilation skipped

Running 22 tests for src/test/BondChainlinkOracle.t.sol:BondChainlinkOracleTest
[PASS] testFuzzRevert_setPair_doubleFeedDiv_add_invalid(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 62164, ~: 63222)
[PASS] testFuzzRevert_setPair_doubleFeedMul_add_invalid(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 61944, ~: 63192)
[PASS] testFuzzRevert_setPair_oneFeedInverse_add_invalid(uint8,uint8,uint48) (runs: 256, μ: 31171, ~: 31824)
[PASS] testFuzzRevert_setPair_oneFeed_add_invalid(uint8,uint8,uint48) (runs: 256, μ: 31274, ~: 31890)
[PASS] testFuzz_currentPrice_doubleFeedDiv(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 929, ~: 932)
[PASS] testFuzz_currentPrice_doubleFeedMul(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 948, ~: 953)
[FAIL. Reason: Arithmetic over/underflow Counterexample: calldata=0x43c2c9ef000000000000000000000000000000000000000000000000000000000000001100000000000000000000000000000000000000000000000000000000000000070000000000000000000000000000000000000000000000000000000000002973, args=[17, 7, 10611]] testFuzz_currentPrice_oneFeed(uint8,uint8,uint48) (runs: 246, μ: 715, ~: 719)
Traces:
  [161261] owner::testFuzz_currentPrice_oneFeed(17, 7, 10611) 
    ├─ [5374] MockPriceFeed::setDecimals(7) 
    │   └─ ← ()
    ├─ [2371] MockPriceFeed::latestAnswer() [staticcall]
    │   └─ ← 8000000000000000000
    ├─ [3159] MockPriceFeed::setLatestAnswer(80000000) 
    │   └─ ← ()
    ├─ [19697] BondChainlinkOracle::setPair(MockERC20: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], MockERC20: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], true, 0x000000000000000000000000c7183455a4c133ae270771860664b6b7ec320bb100000000000000000000000000000000000000000000000000000000000029730000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000110000000000000000000000000000000000000000000000000000000000000000) 
    │   ├─ [364] MockPriceFeed::decimals() [staticcall]
    │   │   └─ ← 7
    │   ├─ emit PairUpdated(quoteToken: MockERC20: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], payoutToken: MockERC20: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], supported: true)
    │   └─ ← ()
    ├─ [22463] MockAggregator::setMarketAuctioneer(0, auctioneer: [0x90D8A270C314ead2D8C025398dFCA118380BFDAA]) 
    │   └─ ← ()
    ├─ [0] VM::prank(auctioneer: [0x90D8A270C314ead2D8C025398dFCA118380BFDAA]) 
    │   └─ ← ()
    ├─ [50147] BondChainlinkOracle::registerMarket(0, MockERC20: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], MockERC20: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) 
    │   ├─ [444] MockAggregator::getAuctioneer(0) [staticcall]
    │   │   └─ ← auctioneer: [0x90D8A270C314ead2D8C025398dFCA118380BFDAA]
    │   ├─ emit MarketRegistered(id: 0, quoteToken: MockERC20: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], payoutToken: MockERC20: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   └─ ← ()
    ├─ [8901] BondChainlinkOracle::currentPrice(0) [staticcall]
    │   ├─ [4723] MockPriceFeed::latestRoundData() [staticcall]
    │   │   └─ ← 1, 80000000, 0, 1608336000, 1
    │   ├─ [364] MockPriceFeed::decimals() [staticcall]
    │   │   └─ ← 7
    │   └─ ← 800000000000000000
    ├─ [371] MockPriceFeed::latestAnswer() [staticcall]
    │   └─ ← 80000000
    ├─ [3353] MockPriceFeed::setRoundId(2) 
    │   └─ ← ()
    ├─ [0] VM::expectRevert(BondOracle_BadFeed(0xc7183455a4C133Ae270771860664b6B7ec320bB1)) 
    │   └─ ← ()
    ├─ [3130] BondChainlinkOracle::currentPrice(0) [staticcall]
    │   ├─ [723] MockPriceFeed::latestRoundData() [staticcall]
    │   │   └─ ← 2, 80000000, 0, 1608336000, 1
    │   └─ ← "BondOracle_BadFeed(0xc7183455a4C133Ae270771860664b6B7ec320bB1)"
    ├─ [443] MockPriceFeed::setRoundId(1) 
    │   └─ ← ()
    ├─ [371] MockPriceFeed::latestAnswer() [staticcall]
    │   └─ ← 80000000
    ├─ [288] MockPriceFeed::setLatestAnswer(0) 
    │   └─ ← ()
    ├─ [0] VM::expectRevert(BondOracle_BadFeed(0xc7183455a4C133Ae270771860664b6B7ec320bB1)) 
    │   └─ ← ()
    ├─ [3024] BondChainlinkOracle::currentPrice(0) [staticcall]
    │   ├─ [723] MockPriceFeed::latestRoundData() [staticcall]
    │   │   └─ ← 1, 0, 0, 1608336000, 1
    │   └─ ← "BondOracle_BadFeed(0xc7183455a4C133Ae270771860664b6B7ec320bB1)"
    ├─ [288] MockPriceFeed::setLatestAnswer(-1) 
    │   └─ ← ()
    ├─ [0] VM::expectRevert(BondOracle_BadFeed(0xc7183455a4C133Ae270771860664b6B7ec320bB1)) 
    │   └─ ← ()
    ├─ [3024] BondChainlinkOracle::currentPrice(0) [staticcall]
    │   ├─ [723] MockPriceFeed::latestRoundData() [staticcall]
    │   │   └─ ← 1, -1, 0, 1608336000, 1
    │   └─ ← "BondOracle_BadFeed(0xc7183455a4C133Ae270771860664b6B7ec320bB1)"
    ├─ [359] MockPriceFeed::setLatestAnswer(80000000) 
    │   └─ ← ()
    └─ ← "Arithmetic over/underflow"

[PASS] testFuzz_currentPrice_oneFeedInverse(uint8,uint8,uint48) (runs: 256, μ: 1298, ~: 657)
[PASS] testFuzz_decimals_doubleFeedDiv(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 909, ~: 911)
[PASS] testFuzz_decimals_doubleFeedMul(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 931, ~: 934)
[PASS] testFuzz_decimals_oneFeed(uint8,uint8,uint48) (runs: 256, μ: 692, ~: 696)
[PASS] testFuzz_decimals_oneFeedInverse(uint8,uint8,uint48) (runs: 256, μ: 677, ~: 681)
[PASS] testFuzz_setAuctioneer(address,address) (runs: 256, μ: 37663, ~: 37632)
[PASS] testFuzz_setPair_doubleFeedDiv_valid(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 929, ~: 932)
[PASS] testFuzz_setPair_doubleFeedMul_add_valid(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 905, ~: 908)
[PASS] testFuzz_setPair_oneFeedInverse_add_valid(uint8,uint8,uint48) (runs: 256, μ: 656, ~: 659)
[PASS] testFuzz_setPair_oneFeed_add_valid(uint8,uint8,uint48) (runs: 256, μ: 670, ~: 674)
[PASS] testFuzz_setPair_onlyOwner(address) (runs: 256, μ: 201943, ~: 201927)
[PASS] test_currentPrice() (gas: 120212)
[PASS] test_decimals() (gas: 104241)
[PASS] test_registerMarket() (gas: 118483)
[PASS] test_setPair_remove() (gas: 69095)
Test result: FAILED. 21 passed; 1 failed; finished in 144.89ms

Failing tests:
Encountered 1 failing test in src/test/BondChainlinkOracle.t.sol:BondChainlinkOracleTest
[FAIL. Reason: Arithmetic over/underflow Counterexample: calldata=0x43c2c9ef000000000000000000000000000000000000000000000000000000000000001100000000000000000000000000000000000000000000000000000000000000070000000000000000000000000000000000000000000000000000000000002973, args=[17, 7, 10611]] testFuzz_currentPrice_oneFeed(uint8,uint8,uint48) (runs: 246, μ: 715, ~: 719)

Recommendation

Use safe math.

Oighty commented 1 year ago

I don't think this is a finding. See response to #3.