SWC-101 Arithmetic Overflow on Current Price. BondChainlinkOracle.t.sol BondChainlinkOracle.sol function test_currentPrice()

Summary

The test to try to get current price for market that hasn't been registered, should fail. But it does not when using integer overflow. 1 call out of 3 calls to the test to Try to get current price for market that hasn't been registered passes and does not revert using integer overflow when it never should. The result is market was registered.

Vulnerability Detail

// In the function
function test_currentPrice() 
// the following line was suffixed with integer overflow.

function test_currentPrice() public {
        // Try to get current price for market that hasn't been registered, should fail
        bytes memory err = abi.encodeWithSignature("BondOracle_MarketNotRegistered(uint256)", 0);
        vm.expectRevert(err);
        oracle.currentPrice(0+1e18); // fat32 ovf

Impact

The test to Try to get current price for market that hasn't been registered, should fail. But it does not fail when using integer overflow. This means that the current price can be manipulated before it is registered. 1 call out of 3 calls to the test to Try to get current price for market that hasn't been registered passes and does not revert using integer overflow when it never should. The result is market was registered. Please see the log file dump below displays in summary.

Code Snippet

Vulnerable code

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

References
https://swcregistry.io/docs/SWC-101

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

function test_currentPrice() public {
        // Try to get current price for market that hasn't been registered, should fail
        bytes memory err = abi.encodeWithSignature("BondOracle_MarketNotRegistered(uint256)", 0);
        vm.expectRevert(err);
        oracle.currentPrice(0+1e18);  // fat32 ovf

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...
[⠔] Compiling 1 files with 0.8.15
[⠑] Solc 0.8.15 finished in 2.29s
Compiler run successful

Running 22 tests for src/test/BondChainlinkOracle.t.sol:BondChainlinkOracleTest
[PASS] testFuzzRevert_setPair_doubleFeedDiv_add_invalid(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 62107, ~: 63210)
[PASS] testFuzzRevert_setPair_doubleFeedMul_add_invalid(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 62275, ~: 63204)
[PASS] testFuzzRevert_setPair_oneFeedInverse_add_invalid(uint8,uint8,uint48) (runs: 256, μ: 31140, ~: 31824)
[PASS] testFuzzRevert_setPair_oneFeed_add_invalid(uint8,uint8,uint48) (runs: 256, μ: 31357, ~: 31890)
[PASS] testFuzz_currentPrice_doubleFeedDiv(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 928, ~: 932)
[PASS] testFuzz_currentPrice_doubleFeedMul(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 950, ~: 953)
[PASS] testFuzz_currentPrice_oneFeed(uint8,uint8,uint48) (runs: 256, μ: 1360, ~: 719)
[PASS] testFuzz_currentPrice_oneFeedInverse(uint8,uint8,uint48) (runs: 256, μ: 652, ~: 657)
[PASS] testFuzz_decimals_doubleFeedDiv(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 907, ~: 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, μ: 1195, ~: 681)
[PASS] testFuzz_setAuctioneer(address,address) (runs: 256, μ: 37667, ~: 37635)
[PASS] testFuzz_setPair_doubleFeedDiv_valid(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 928, ~: 932)
[PASS] testFuzz_setPair_doubleFeedMul_add_valid(uint8,uint8,uint48,uint8,uint48) (runs: 256, μ: 902, ~: 908)
[PASS] testFuzz_setPair_oneFeedInverse_add_valid(uint8,uint8,uint48) (runs: 256, μ: 655, ~: 659)
[PASS] testFuzz_setPair_oneFeed_add_valid(uint8,uint8,uint48) (runs: 256, μ: 670, ~: 674)
[PASS] testFuzz_setPair_onlyOwner(address) (runs: 256, μ: 201943, ~: 201927)
[FAIL. Reason: Error != expected error: 0x436225170000000000000000000000000000000000000000000000000de0b6b3a7640000 != Cb%] test_currentPrice() (gas: 13310)
Traces:
  [13310] owner::test_currentPrice() 
    ├─ [0] VM::expectRevert(BondOracle_MarketNotRegistered(0)) 
    │   └─ ← ()
    ├─ [4872] BondChainlinkOracle::currentPrice(1000000000000000000) [staticcall]
    │   └─ ← "BondOracle_MarketNotRegistered(1000000000000000000)"
    └─ ← "Error != expected error: 0x436225170000000000000000000000000000000000000000000000000de0b6b3a7640000 != Cb%"

[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 138.02ms

Failing tests:
Encountered 1 failing test in src/test/BondChainlinkOracle.t.sol:BondChainlinkOracleTest
[FAIL. Reason: Error != expected error: 0x436225170000000000000000000000000000000000000000000000000de0b6b3a7640000 != Cb%] test_currentPrice() (gas: 13310)

Encountered a total of 1 failing tests, 21 tests succeeded

Recommendation

Use safe math.

sherlock-audit / 2023-02-bond-judging

fat32 - SWC-101 Arithmetic Overflow on Current Price. BondChainlinkOracle.t.sol BondChainlinkOracle.sol function test_currentPrice() #10