Short fuzzing exercise

README.md

Fuzzing Exercise

This is a short and simple exercise to start sharpening your smart contract fuzzing skills with Foundry.

The scenario is simple. There's a registry contract that allows callers to register by paying a fixed fee in ETH. If the caller sends too little ETH, execution should revert. If the caller sends too much ETH, the contract should give back the change.

Things look good according to the unit test we coded in the Registry.t.sol contract.

Your goal is to code at least one fuzz test for the Registry contract. By following the brief specification above, the test must be able to detect a bug in the register function.

Registry.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

contract Registry {
    error PaymentNotEnough(uint256 expected, uint256 actual);

    uint256 public constant PRICE = 1 ether;

    mapping(address account => bool registered) private registry;

    function register() external payable {
        if(msg.value < PRICE) {
            revert PaymentNotEnough(PRICE, msg.value);
        }

        registry[msg.sender] = true;
    }

    function isRegistered(address account) external view returns (bool) {
        return registry[account];
    }
}

Registry.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";
import {Registry} from "../src/Registry.sol";

contract RegistryTest is Test {
    Registry registry;
    address alice;

    function setUp() public {
        alice = makeAddr("alice");
        
        registry = new Registry();
    }

    function test_register() public {
        uint256 amountToPay = registry.PRICE();
        
        vm.deal(alice, amountToPay);
        vm.startPrank(alice);

        uint256 aliceBalanceBefore = address(alice).balance;

        registry.register{value: amountToPay}();

        uint256 aliceBalanceAfter = address(alice).balance;
        
        assertTrue(registry.isRegistered(alice), "Did not register user");
        assertEq(address(registry).balance, registry.PRICE(), "Unexpected registry balance");
        assertEq(aliceBalanceAfter, aliceBalanceBefore - registry.PRICE(), "Unexpected user balance");
    }

    /** Code your fuzz test here */
}

Registry.t.solved.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";
import {Registry} from "../src/Registry.sol";

contract RegistryTest is Test {
    Registry registry;
    address alice;

    function setUp() public {
        alice = makeAddr("alice");
        
        registry = new Registry();
    }

    function test_register() public {
        uint256 amountToPay = registry.PRICE();
        
        vm.deal(alice, amountToPay);
        vm.startPrank(alice);

        uint256 aliceBalanceBefore = address(alice).balance;

        registry.register{value: amountToPay}();

        uint256 aliceBalanceAfter = address(alice).balance;
        
        assertTrue(registry.isRegistered(alice), "Did not register user");
        assertEq(address(registry).balance, registry.PRICE(), "Unexpected registry balance");
        assertEq(aliceBalanceAfter, aliceBalanceBefore - registry.PRICE(), "Unexpected user balance");
    }

    /** Almost the same test, but this time fuzzing amountToPay detects the bug (the Registry contract is not giving back the change) */
    function test_fuzz_register(uint256 amountToPay) public {
        vm.assume(amountToPay >= 1 ether);
        
        vm.deal(alice, amountToPay);
        vm.startPrank(alice);

        uint256 aliceBalanceBefore = address(alice).balance;

        registry.register{value: amountToPay}();

        uint256 aliceBalanceAfter = address(alice).balance;
        
        assertTrue(registry.isRegistered(alice), "Did not register user");
        assertEq(address(registry).balance, registry.PRICE(), "Unexpected registry balance");
        assertEq(aliceBalanceAfter, aliceBalanceBefore - registry.PRICE(), "Unexpected user balance");
    }
}

TEST

    function testIsTooHigh_registerTest(uint amountToTest) public{
        vm.assume(amountToTest >= 1 ether);
        uint registeringAmount = registry.PRICE();
        vm.startPrank(alice);
        vm.deal(alice, amountToTest);
        uint256 startingAliceBalance = address(alice).balance;
        
        registry.register{value: amountToTest}();
        uint256 endingAliceBalance = address(alice).balance;

        assertEq (endingAliceBalance, startingAliceBalance - registry.PRICE());
    }

ERROR

    
proptest: Aborting shrinking after the PROPTEST_MAX_SHRINK_ITERS environment variable or ProptestConfig.max_shrink_iters iterations (set 0 to a large(r) value to shrink more; current configuration: 0 iterations)
proptest: Saving this and future failures in cache/fuzz/failures
proptest: If this test was run on a CI system, you may wish to add the following line to your copy of the file. (You may need to create it.)
cc 4bc18a146c42061b4ef475bac06b2dc24f8b3a8ca815b446a21052d9e1edb0e9

Ran 1 test for test/challengeTest.t.sol:RegistryTest
[FAIL; counterexample: calldata=0x8f29e5bdfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffd args=[115792089237316195423570985008687907853269984665640564039457584007913129639933 [1.157e77]]] testIsTooHigh_registerTest(uint256) (runs: 0, μ: 0, ~: 0)
Logs:
  Error: a == b not satisfied [uint]
        Left: 0
       Right: 115792089237316195423570985008687907853269984665640564039456584007913129639933

⚠️ Balance is not returned when users send more than the Registry::PRICE which leads to loss of funds.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";
import {Registry} from "../src/Registry.sol";

contract RegistryTest is Test {
    Registry registry;
    address alice;

    function setUp() public {
        alice = makeAddr("alice");

        registry = new Registry();
    }

    function test_register() public {
        uint256 amountToPay = registry.PRICE();

        vm.deal(alice, amountToPay);
        vm.startPrank(alice);

        uint256 aliceBalanceBefore = address(alice).balance;

        registry.register{value: amountToPay}();

        uint256 aliceBalanceAfter = address(alice).balance;

        assertTrue(registry.isRegistered(alice), "Did not register user");
        assertEq(
            address(registry).balance,
            registry.PRICE(),
            "Unexpected registry balance"
        );
        assertEq(
            aliceBalanceAfter,
            aliceBalanceBefore - registry.PRICE(),
            "Unexpected user balance"
        );
    }

    function test_revertIfFeeIsLessThanPrice(uint256 _fee) public {
        uint256 PRICE = registry.PRICE();

        vm.assume(_fee < PRICE);

        vm.deal(alice, _fee);
        vm.startPrank(alice);

        vm.expectRevert();
        registry.register{value: _fee}();
    }

    function test_returnBalanceIfFeeIsGreaterThanPrice(uint256 _fee) public {
        uint256 PRICE = registry.PRICE();

        vm.assume(_fee >= PRICE);

        vm.deal(alice, _fee);
        vm.startPrank(alice);

        uint256 aliceBalanceBefore = address(alice).balance;

        registry.register{value: _fee}();

        uint256 aliceBalanceAfter = address(alice).balance;

        assertEq(
            address(registry).balance,
            PRICE,
            "Unexpected registry balance"
        );
        assertEq(
            aliceBalanceAfter,
            aliceBalanceBefore - PRICE,
            "Unexpected user balance"
        );
    }
}

function test_fuzz_register_does_not_send_extra_eth_back(uint256 fuzzAmount) public{ fuzzAmount = bound(fuzzAmount,registry.PRICE(),10 ether); vm.deal(alice,fuzzAmount); vm.startPrank(alice); uint256 sentBackAmount; if(fuzzAmount > registry.PRICE()){ sentBackAmount = fuzzAmount - registry.PRICE(); } uint256 aliceBalanceBefore = address(alice).balance; registry.register{value: fuzzAmount}(); uint256 aliceBalanceAfter = address(alice).balance; assertEq(aliceBalanceAfter,sentBackAmount,"Did not send back ether"); }

[H-1] The function Registry:register, do not give back the change, if a user send more than the registration price (1 ether), causing the lose of the extra ether that user sent.

Description: The function Register:register, if a user send more than 1 ether that is the registration price, the function won't give back the extra ether to the user, because in the function register, the protocol doesn't have a way to give back the extra ether to the user.

?        function register() external payable {
?        if(msg.value < PRICE) {
?            revert PaymentNotEnough(PRICE, msg.value);
?        }
?
?        registry[msg.sender] = true;
?    }

Impact:

The extra ether sent by the user will be lost, because them won't be returned to the user, and the contract don't have a way to withdraw ether.

Proof of Concept:

1. A user send 1,5 ether to the contract.
2. The registration cost is 1 Ether so the user sent 0,5 ether extra.
3. The function will be executed and the user will be registered.
4. These 0,5 ether will be lost.

To a more technical explanation, you can paste this test into Registry.t.sol:

Prove of Code (PoC)

Here you have the simplier PoF code that i could write:

    function test_register_dont_give_back_the_change_no_fuzz() public {
        uint256 amountToPay = 2 ether;

        vm.deal(alice, amountToPay);
        uint256 aliceBalanceBefore = address(alice).balance;
        vm.prank(alice);
        registry.register{value: amountToPay}();
        uint256 aliceBalanceAfter = address(alice).balance;

        assertEq(registry.isRegistered(alice), true, "Did not register user");
        assertEq(address(registry).balance, registry.PRICE(), "Unexpected registry balance");
        assertEq(aliceBalanceAfter, aliceBalanceBefore - registry.PRICE(), "Unexpected user balance");
    }

and here you have the fuzzer test that i could write:

    function test_register_fuzz (uint256 amountToPay) public {
        vm.assume(amountToPay >= registry.PRICE());
        vm.deal(alice, amountToPay);
        uint256 aliceBalanceBefore = address(alice).balance;
        vm.prank(alice);
        registry.register{value: amountToPay}();
        uint256 aliceBalanceAfter = address(alice).balance;

        assertEq(registry.isRegistered(alice), true, "Did not register user");
        assertEq(address(registry).balance, registry.PRICE(), "Unexpected registry balance");
        assertEq(aliceBalanceAfter, aliceBalanceBefore - registry.PRICE(), "Unexpected user balance");
    }

Recommendation:

1. After making the registration, transfer the extra ether to the user. (what natspec says)
2. Add some validation before the registration, to revert if the user sent more or less ether than the expected.
3. Add a function to withdraw the extra ether.

To return the extra ether to the user automatically, you can add this to the function register:

    uint256 extraEther = msg.value - PRICE;
    (bool success, ) = payable(msg.sender).call{value: extraEther}("");
    if(!success) {
        revert;
    }

To add some extra validation before the registration, you can do this on the function register:

-    if(msg.value < PRICE) {
-        revert PaymentNotEnough(PRICE, msg.value);
-    }

+    if(msg.value != PRICE) {
+        revert;
+    }

What i'm doing here is to check if the ether sent by the user is diferent than the expected, it will revert, not just revert if it is less than the expected.

Since others put their solutions here I figured I might as well put mine. It was a nice exercise to help me practice writing fuzz tests. Thanks Tincho!

function testFuzzRegister(uint256 amountToPay) public {
        uint256 costToRegister = registry.PRICE();
        vm.deal(alice, 100e18);

        amountToPay = bound(amountToPay, costToRegister, address(alice).balance);
        if (address(alice).balance < costToRegister) {
            return;
        }

        uint256 aliceBalanceBefore = address(alice).balance;

        vm.startPrank(alice);
        registry.register{value: amountToPay}();

        uint256 aliceBalanceAfter = address(alice).balance;
        uint256 expectedBalance = aliceBalanceBefore - costToRegister;

        assertEq(aliceBalanceAfter, expectedBalance);
}

Completed

Challenge completed.
This very likely to be a HIGH vulnerability, in my opinion likelihood is medium because not every call of function will be with big msg.value, but impact is big, there is not an amount of upper border that user can send, so lots of money can be stolen from user.
Proof of Code:

     function test_register_detect_bug(uint256 startingBalance) public {
        uint256 registerFee = registry.PRICE();
        startingBalance = bound(startingBalance, registerFee, type(uint64).max);

        vm.deal(alice, startingBalance);
        vm.startPrank(alice);

        registry.register{value: startingBalance}();

        console2.log("Sent value is: ", startingBalance);
        console2.log("Left value after `register` function is: ", alice.balance);

        // We are trying to assert that the `register` function gets only needed price of entering.
        // If we sent more than fee price the function should return the `change`
        // So user balance at he end should be equal to value that we sent to the function subtracted by register price
        // Otherwise we are catching a bug
        assert(alice.balance == startingBalance - registerFee);
    }