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");
    }
}

Thank you Tincho! Here is my Fuzz output, did it a little differently.

 function test_RegisterFuzz_AN(uint256 randomnumber) public {
        uint256 amountToPay = registry.PRICE();
        uint256 randomAmount = amountToPay * randomnumber;

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

        uint256 aliceBalanceBefore = address(alice).balance;
        registry.register{value: randomAmount}();
        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");
    }
    

Error Output

Ran 1 test for test/Registry.t.sol:RegistryTest
[FAIL. Reason: Unexpected registry balance: 78620360604071275417471593677350105936000000000000000000 != 1000000000000000000; counterexample: calldata=0x19ff63be000000000000000000000000000000003b25bb0d5bdf9a3e5945a0424789f350 args=[78620360604071275417471593677350105936 [7.862e37]]] test_RegisterFuzz_AN(uint256) (runs: 0, μ: 0, ~: 0)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.15ms (531.60µs CPU time)

function testRegisterFuzz(uint256 amount) public {
//Arrange
vm.assume(amount >= 1 ether);
uint256 AliceAmount = amount;

//act

vm.deal(alice, AliceAmunt);
vm.prank(alice);

uint256 AliceBalanceBefore = address(alice).balance
registery.register{value: AliceAmount}();
uint256 AliceBalanceAfter = address(alice).balance

//assert

assertTrue(registry.isRegisterd(alice), "alice has entered the registry");
assertEq(address(registry).balance, registery.PRICE(), "unexpected registery balance");
assertEq(AliceBalanceAfter, AliceBalanceBefore - registery.PRICE(), "unexpected registery balance")

}

i have setup the user in the setUp() function and gave the user balance.(not included in the below file). I hope this is correct

function testRegister(uint256 value) public {
uint256 UserBalanceBefore = address(user).balance;
uint256 ogPrice = registry.PRICE();
value = bound(value, ogPrice, 100 ether);
vm.startPrank(user);
registry.register{ value: value }();
uint256 UserBalanceAfter = address(user).balance;
assertEq(registry.isRegistered(user), true);
assertEq((UserBalanceBefore - ogPrice), UserBalanceAfter);
assertEq(address(registry).balance, ogPrice);
vm.stopPrank();
}

[H-1] Potential loss of funds can occur if the user exceeds the fixed price of 1e18

Description When invoking Registry::register, a participant can exceed the fixed price of 1e18, as there are no additional checks to verify if the price has been exceeded or no actions to send the user their change.

Impact The participant's balance is not refunded when exceeding the fixed price of 1e18, potentially resulting in a loss of funds

Proof of Concepts

    function test_fuzzer(uint256 amount) public {
        amount = bound(amount, 2e18, 10e18);
        vm.deal(alice, amount);
        console.log("Alice's pre balance: %s", address(alice).balance);
        
        vm.prank(alice);
        registry.register{value: amount}();
        console.log("Registry's post balance: %s", address(registry).balance);

        // registry balance should not exceed the fixed price of 1e18 
        assert(address(registry).balance > 1e18);
        // no change is given back to the sender after exceeding the entry fixed price of 1e18 
        console.log("Alice's post balance: %s", address(alice).balance);
    }

    [47154] RegistryTest::test_fuzzer(43)
        ├─ [0] console::log("Bound result", 8000000000000000044 [8e18]) [staticcall]
        │   └─ ← [Stop] 
        ├─ [0] VM::deal(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], 8000000000000000044 [8e18])
        │   └─ ← [Return] 
        ├─ [0] console::log("Alice's pre balance: %s", 8000000000000000044 [8e18]) [staticcall]
        │   └─ ← [Stop] 
        ├─ [0] VM::prank(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6])
        │   └─ ← [Return] 
        ├─ [22318] 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f::register{value: 8000000000000000044}()
        │   └─ ← [Stop] 
        ├─ [0] console::log("Registry post balance: %s", 8000000000000000044 [8e18]) [staticcall]
        │   └─ ← [Stop] 
        ├─ [0] console::log("Alice's post balance: %s", 0) [staticcall]
        │   └─ ← [Stop] 
        └─ ← [Stop] 

    Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 22.74ms (21.18ms CPU time)

Recommended Mitigation Prevent the entire transaction from going through if msg.value exceeds 1e18.

+   error Registry__PaymentExceedsPrice(uint256 expected, uint256 actual);
    
    function register() external payable {
        if(msg.value < PRICE) {
            revert PaymentNotEnough(PRICE, msg.value);
        }
+       if (msg.value > PRICE) { 
+           revert Registry__PaymentExceedsPrice(PRICE, msg.value); 
+       }

       registry[msg.sender] = true;
    }

Alternatively, you can implement a refund mechanism that forwards the remaining amount to msg.sender while registering the participant:

+ error Registry__RefundFailed(uint256 amount);

    function register() external payable {
        if(msg.value < PRICE) {
            revert PaymentNotEnough(PRICE, msg.value);
        }
+       if (msg.value > PRICE) { 
+       uint256 refundAmount = msg.value - PRICE;
+       (bool ok, ) = msg.sender.call{value: refundAmount}("");
+           if (!ok) { revert Registry__RefundFailed(refundAmount); }
+        }

        registry[msg.sender] = true;    
    }
  

BUG: LOCK UP OF FUNDS IN CONTRACT

This bug comes about anytime the user sends more than the price needed to register. When User does this the excess fees are not returned back to user as intended but rather stay locked up in the contract.

THE TEST

This is the fuzz test that caught the bug

    /**
     * Solution to the challenge1
     * Fuzzing the amount parameter to find edge cases
     * Bug is explained in `testFeeOverSent`
     * @param amount the  parameter to fuzz
     */
    function testFuzzRegister(uint256 amount) public {
        amount = bound(amount,1e18,type(uint128).max);

        uint256 bal1 = address(0xabc).balance;

        vm.prank(address(0xabc));
        reg.register{value: amount}();

        uint256 bal2 = address(0xabc).balance;

        assertTrue(reg.isRegistered(address(0xabc)));
        assertEq(bal1-bal2, 1 ether);
        assertEq(address(reg).balance, 1 ether);

    }

• From the fuzz Test I discovered that:
  • whenever the fees are over sent it does not refund as intended so it Inflates the Register contract balance.
  • Ether becomes stuck because there is no way to transfer it back.

The test below explains the bug caught

 function testFeeOverSent() public {
        vm.prank(address(0xabc));

        console2.log("Balance one: ",address(0xabc).balance);

        reg.register{value:12e18}();

        console2.log("Balance two: ",address(0xabc).balance);

        assertTrue(reg.isRegistered(address(0xabc)));
    }

So from the test above we'll see that the transfer does not revert and the address is registered
But from the balances logged we'll notice that the excess funds were not returned as caught in the fuzz test.

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