[Bug-Candidate]: ABDKMath64x64PropertyTests.sol: Tests output false negatives
- Dominant language
- Solidity
- Stars
- 371
- Forks
- 59
- PR merge metrics
- No merged PRs in 30d
Description
### Describe the issue:
These 2 tests:
```java
contract CryticABDKMath64x64Properties {
...
function add_test_range(int128 x, int128 y) public view {
int128 result;
try this.add(x, y) {
result = this.add(x, y);
assert(result <= MAX_64x64 && result >= MIN_64x64);
} catch {
// If it reverts, just ignore
}
}
function sub_test_range(int128 x, int128 y) public view {
int128 result;
try this.sub(x, y) {
result = this.sub(x, y);
assert(result <= MAX_64x64 && result >= MIN_64x64);
} catch {
// If it reverts, just ignore
}
}
}
```
They are not actually showing failure in case `this.add`/`this.sub` overflow or underflow.
It looks like `result` being int128 makes it always conforming with the assert statement despite the failure in `this.add`/`this.sub`.
### Steps to reproduce the issue:
- Create a smart contract to contain the test in ``tests/ABDKMath64x64PropertyTests/hardhat/contracts/CryticMathTest.sol`
```java
pragma solidity ^0.8.0;
import "@crytic/properties/contracts/Math/ABDKMath64x64/ABDKMath64x64PropertyTests.sol";
contract CryticABDKMath64x64Harness is CryticABDKMath64x64Properties {
/* Any additional test can be added here */
}
```
- Working directory: `tests/ABDKMath64x64PropertyTests/hardhat`
- Force `ABDKMath64x64` to overflow/underflow by commenting out the require statement on the tested methods.
```java
library ABDKMath64x64 {
....
function add (int128 x, int128 y) internal pure returns (int128) {
unchecked {
int256 result = int256(x) + y;
// COMMENTED THIS TO FORCE IT TO FAIL
// require (result >= MIN_64x64 && result <= MAX_64x64);
return int128 (result);
}
}
function sub (int128 x, int128 y) internal pure returns (int128) {
unchecked {
int256 result = int256(x) - y;
// COMMENTED THIS TO FORCE IT TO FAIL
// require (result >= MIN_64x64 && result <= MAX_64x64);
return int128 (result);
}
}
}
```
- Run `echidna-test . --contract CryticABDKMath64x64Harness --test-mode assertion`
- Output:
```
sub_test_range(int128,int128): passed! 🎉
abs(int128): passed! 🎉
add_test_maximum_value(): passed! 🎉
mulu(int128,uint256): passed! 🎉
add_test_range(int128,int128): passed! 🎉
```
- Some other tests failed due to that change but `add_test_range` and `sub_test_range` did not fail as expected.
### If additional code is needed for reproducing, please copy it here, or drop us a link to the repository:
_No response_
### Echidna version:
Echidna 2.0.1
### Additional information:
_No response_
Contributor guide
Research direction
Start with tests/ABDKMath64x64PropertyTests/hardhat/contracts/CryticMathTest.sol and the ABDKMath64x64PropertyTests.sol definitions, then run echidna-test . --contract CryticABDKMath64x64Harness --test-mode assertion. Reproduce the issue with the overflow checks disabled in ABDKMath64x64. Done means add_test_range and sub_test_range detect the forced overflow or underflow instead of passing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- solidity
- Domain
- security, testing-qa
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100