GivingThanks
First Flight #28
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

New Tests: Lack of Comprehensive Testing Coverage and Additional Tests Added in GivingThanks.t.sol

koinerdegen

Summary:
The GivingThanks smart contract previously had limited testing coverage, missing several critical scenarios that could lead to potential issues or unexpected behaviors in production. By adding more comprehensive tests, we improved the contract's robustness and helped identify potential vulnerabilities. This report outlines the importance of these new tests and highlights the improvements made.

Vulnerability Details:
The original test suite for GivingThanks.t.sol lacked coverage for several critical functions and edge cases, including:

  1. Access Control for updateRegistry: Tests did not include scenarios for unauthorized access attempts to updateRegistry.

  2. Donations to Unverified Charities: Edge cases were not sufficiently tested for donating to unverified charities.

  3. Token Minting Edge Cases: The minting process was not tested thoroughly, particularly in cases where the recipient might be a smart contract not supporting ERC721 tokens.

  4. Event Emissions: There were no tests verifying the correct emission of events, such as DonateToCharity.

  5. Address(0) Checks: Scenarios where invalid addresses (e.g., address(0)) were passed to functions were not covered.

Impact:
The lack of comprehensive testing could lead to:

  • Unexpected Contract Behavior: Missed edge cases and incorrect assumptions can cause bugs to surface during deployment or interaction with users.

  • Security Vulnerabilities: Limited testing increases the risk of undetected issues, including potential exploits related to access control or improper function inputs.

  • Reduced Confidence in Smart Contract: Incomplete testing reduces the confidence developers and users can have in the reliability of the contract, possibly impacting adoption and usage.

Tools Used:

  • Foundry for running tests (forge test)

  • Coverage Analysis using Foundry’s built-in coverage tool (forge coverage)

Recommendations:
Additional Tests Added

Test Breakdown and Explanations:

Detailed Test Functions for CharityRegistry.sol

1. testCharityRegistryChangeAdminWithAdmin

function testCharityRegistryChangeAdminWithAdmin() public {
address newAdmin = makeAddr("newAdmin");
​
vm.startPrank(admin);
registryContract.changeAdmin(newAdmin);
vm.stopPrank();
address updatedAdmin = registryContract.admin();
​
assertEq(newAdmin, updatedAdmin);
}
​

  • Purpose: Verifies that only the current admin can change the admin address.

  • Explanation:

    • Sets up a new address (newAdmin).

    • Impersonates the current admin using vm.startPrank(admin).

    • Calls changeAdmin to update the admin address.

    • Asserts that the admin address has been successfully updated to newAdmin.

2. testCharityRegistryChangeAdminWithAdminRevert

function testCharityRegistryChangeAdminWithAdminRevert() public {
address newAdmin = makeAddr("newAdmin");
​
vm.startPrank(donor);
vm.expectRevert("Only admin can change admin");
registryContract.changeAdmin(newAdmin);
vm.stopPrank();
}

  • Purpose: Ensures that a non-admin user cannot change the admin address.

  • Explanation:

    • Sets up a new address (newAdmin).

    • Impersonates a non-admin user (donor).

    • Expects the call to changeAdmin to revert with the message "Only admin can change admin".

3. testCharityVerifyCharityAdminWithAdmin

function testCharityVerifyCharityAdminWithAdmin() public {
address newCharity = makeAddr("newCharity");
vm.startPrank(newCharity);
registryContract.registerCharity(newCharity);
vm.stopPrank();
​
vm.startPrank(admin);
registryContract.verifyCharity(newCharity);
vm.stopPrank();
assert(registryContract.isVerified(newCharity));
}

  • Purpose: Confirms that only the admin can verify a registered charity.

  • Explanation:

    • Registers a new charity (newCharity).

    • Impersonates the admin and calls verifyCharity to verify the charity.

    • Asserts that the charity is now verified (isVerified(newCharity) returns true).

4. testCharityVerifyCharityAdminWithAdminRevert

function testCharityVerifyCharityAdminWithAdminRevert() public {
address newCharity = makeAddr("newCharity");
vm.startPrank(newCharity);
registryContract.registerCharity(newCharity);
vm.stopPrank();
​
vm.startPrank(donor);
vm.expectRevert("Only admin can verify");
registryContract.verifyCharity(newCharity);
vm.stopPrank();
}

  • Purpose: Ensures that a non-admin user cannot verify a charity.

  • Explanation:

    • Registers a new charity (newCharity).

    • Impersonates a non-admin user (donor) and attempts to call verifyCharity.

    • Expects the call to revert with the message "Only admin can verify".

5. testCharityVerifyCharityAdminWithAdminRevertNotRegisteredCharities

function testCharityVerifyCharityAdminWithAdminRevertNotRegisteredCharities() public {
address newCharity = makeAddr("newCharity");
​
vm.startPrank(admin);
vm.expectRevert("Charity not registered");
registryContract.verifyCharity(newCharity);
vm.stopPrank();
}

  • Purpose: Tests that a charity cannot be verified if it was not registered first.

  • Explanation:

    • Creates a new charity address (newCharity) without registering it.

    • Impersonates the admin and attempts to verify the charity.

    • Expects the call to revert with the message "Charity not registered".

Detailed Test Functions for GivingThanks.sol

1. testGivingThanksUpdateRegistryNotProtected

function testGivingThanksUpdateRegistryNotProtected() public {
address oldRegistryAddress = address(charityContract.registry());
​
address attackerAddress = makeAddr("attackerAddress");
vm.startPrank(attackerAddress);
charityContract.updateRegistry(attackerAddress);
vm.stopPrank();
​
address updatedRegistryAddress = address(charityContract.registry());
​
assertNotEq(oldRegistryAddress, updatedRegistryAddress);
}

  • Purpose: Checks if the updateRegistry function can be called by anyone, demonstrating the lack of access control.

  • Explanation:

    • Stores the current registry address.

    • Impersonates an attacker and calls updateRegistry with a new address.

    • Asserts that the registry address has changed, indicating the function is unprotected.

2. testGivingThanksCreateTokenURIReturnCorrectValues

function testGivingThanksCreateTokenURIReturnCorrectValues() public {
uint256 donationAmount = 1 ether;
uint256 initialTokenCounter = charityContract.tokenCounter();
​
vm.deal(donor, 10 ether);
vm.prank(donor);
​
charityContract.donate{value: donationAmount}(charity);
​
uint256 newTokenCounter = charityContract.tokenCounter();
assertEq(newTokenCounter, initialTokenCounter + 1);
​
address ownerOfToken = charityContract.ownerOf(initialTokenCounter);
assertEq(ownerOfToken, donor);
​
uint256 charityBalance = charity.balance;
assertEq(charityBalance, donationAmount);
​
console.log(block.timestamp);
console.log(donor);
string memory resFromFunction = charityContract._createTokenURI(
donor,
block.timestamp,
1 ether
);
​
console.log(resFromFunction);
​
string memory checkURI = charityContract.tokenURI(0);
console.log(checkURI);
​
assertEq(checkURI, resFromFunction);
}

  • Purpose: Verifies that the _createTokenURI function generates the correct token URI and ensures the donation process works properly.

  • Explanation:

    • Sets up a donation amount and retrieves the initial token counter.

    • Funds the donor and impersonates them for the donation.

    • Asserts that the token counter has incremented by 1 and checks the owner of the new token.

    • Verifies the charity's balance to ensure the donation was received.

    • Calls _createTokenURI and compares the returned URI with the stored value (tokenURI(0)).

Conclusion:

forge coverage before adding new test functions:

Ran 3 tests for test/GivingThanks.t.sol:GivingThanksTest
[PASS] testCannotDonateToUnverifiedCharity() (gas: 53483)
[PASS] testDonate() (gas: 303480)
[PASS] testFuzzDonate(uint96) (runs: 257, μ: 306495, ~: 305270)
Suite result: ok. 3 passed; 0 failed; 0 skipped; finished in 162.65ms (145.66ms CPU time)
​
Ran 1 test suite in 166.51ms (162.65ms CPU time): 3 tests passed, 0 failed, 0 skipped (3 total tests)
| File | % Lines | % Statements | % Branches | % Funcs |
|-------------------------|----------------|----------------|---------------|--------------|
| src/CharityRegistry.sol | 75.00% (6/8) | 75.00% (6/8) | 33.33% (2/6) | 80.00% (4/5) |
| src/GivingThanks.sol | 92.86% (13/14) | 94.12% (16/17) | 75.00% (3/4) | 75.00% (3/4) |
| Total | 86.36% (19/22) | 88.00% (22/25) | 50.00% (5/10) | 77.78% (7/9) |
​

forge coverage after adding new test functions:

Ran 10 tests for test/GivingThanks.t.sol:GivingThanksTest
[PASS] testCannotDonateToUnverifiedCharity() (gas: 53571)
[PASS] testCharityRegistryChangeAdminWithAdmin() (gas: 20954)
[PASS] testCharityRegistryChangeAdminWithAdminRevert() (gas: 16773)
[PASS] testCharityVerifyCharityAdminWithAdmin() (gas: 64488)
[PASS] testCharityVerifyCharityAdminWithAdminRevert() (gas: 41014)
[PASS] testCharityVerifyCharityAdminWithAdminRevertNotRegisteredCharities() (gas: 19061)
[PASS] testDonate() (gas: 303370)
[PASS] testFuzzDonate(uint96) (runs: 257, μ: 306479, ~: 305335)
[PASS] testGivingThanksCreateTokenURIReturnCorrectValues() (gas: 349021)
[PASS] testGivingThanksUpdateRegistryNotProtected() (gas: 20592)
Suite result: ok. 10 passed; 0 failed; 0 skipped; finished in 165.03ms (184.87ms CPU time)
​
Ran 1 test suite in 165.37ms (165.03ms CPU time): 10 tests passed, 0 failed, 0 skipped (10 total tests)
| File | % Lines | % Statements | % Branches | % Funcs |
|-------------------------|-----------------|-----------------|---------------|---------------|
| src/CharityRegistry.sol | 100.00% (8/8) | 100.00% (8/8) | 100.00% (6/6) | 100.00% (5/5) |
| src/GivingThanks.sol | 100.00% (14/14) | 100.00% (17/17) | 75.00% (3/4) | 100.00% (4/4) |
| Total | 100.00% (22/22) | 100.00% (25/25) | 90.00% (9/10) | 100.00% (9/9) |
​
Updates

Lead Judging Commences

n0kto Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.