Relevant Links
Summary
Inside the GivingThanks::constructor, the contract's CharityRegistry registry state variable is incorrectly assigned to CharityRegistry(msg.sender), causing any updates to the original registry contract's state (such as newly verified charities), to be completely ignored.
Vulnerability Details
Inside the GivingThanks::constructor, the parameter address _registry is passed in as a way to assign the contract's CharityRegistry in order to keep track of what charity is verified and which ones aren't. However, instead of assigning this parameter to the contract's state variable CharityRegistry registry, CharityRegistry(msg.sender) is erroneously used as seen below:
constructor(address _registry) ERC721("DonationReceipt", "DRC") {
@> registry = CharityRegistry(msg.sender);
owner = msg.sender;
tokenCounter = 0;
}
Because of this incorrect assignment, any charity that was previously verified before contract initialization, will be ignored and therefore, are unable to recieve any donations provided by any donors.
Impact
Likelihood: High/Medium
State variables are a critical component of smart contracts, storing important data that affects the contract's behavior and security. Because of this erroneous assignment, any interaction with the contract's main function donate will revert, due to the given charity's verified data not being present.
Severity: High/Medium
Any donors who wish to donate any funds to a given charity will have their transactions revert, since the charity won't be present in the verifiedCharities mapping. This means any funds won't being able to reach the given charity.
POC
POC - 1: Original Test Failure
The first proof of code that shows this is an issue, comes from two of the three tests that are implemented inside /test/GivingThanks.t.sol:
testDonate()
testFuzzDonate()
Both of these tests fail when calling on GivingThanks::donate() due to the require as seen in the output of both tests.
To verify the output of testDonate, please run the command:
forge test --mt testDonate -vvv
Output
No files changed, compilation skipped
Ran 1 test for test/GivingThanks.t.sol:GivingThanksTest
[FAIL. Reason: EvmError: Revert] testDonate() (gas: 27707)
Traces:
[27707] GivingThanksTest::testDonate()
├─ [2383] GivingThanks::tokenCounter() [staticcall]
│ └─ ← [Return] 0
├─ [0] VM::deal(donor: [0xBd4D11D884c3c7bF373b013AC52fd99f9DD86D0A], 10000000000000000000 [1e19])
│ └─ ← [Return]
├─ [0] VM::prank(donor: [0xBd4D11D884c3c7bF373b013AC52fd99f9DD86D0A])
│ └─ ← [Return]
├─ [5256] GivingThanks::donate{value: 1000000000000000000}(charity: [0x59cf1Fbe6AD1EBf1a01e78EE808B7c889E6dd58A])
│ ├─ [0] admin::isVerified(charity: [0x59cf1Fbe6AD1EBf1a01e78EE808B7c889E6dd58A]) [staticcall]
│ │ └─ ← [Stop]
│ └─ ← [Revert] EvmError: Revert
└─ ← [Revert] EvmError: Revert
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 863.35µs (78.81µs CPU time)
Ran 1 test suite in 4.74ms (863.35µs CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/GivingThanks.t.sol:GivingThanksTest
[FAIL. Reason: EvmError: Revert] testDonate() (gas: 27707)
Encountered a total of 1 failing tests, 0 tests succeeded
And for testFuzzDonate(), please run the following command:
forge test --mt testFuzzDonate -vvv
Output
No files changed, compilation skipped
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)
Ran 1 test for test/GivingThanks.t.sol:GivingThanksTest
[FAIL. Reason: EvmError: Revert; counterexample: calldata=0x06a6cd8900000000000000000000000000000000000000000000000000000226c25d4b2d args=[2365492906797 [2.365e12]]] testFuzzDonate(uint96) (runs: 0, μ: 0, ~: 0)
Logs:
Bound result 2365492906797
Traces:
[33991] GivingThanksTest::testFuzzDonate(2365492906797 [2.365e12])
├─ [0] console::log("Bound result", 2365492906797 [2.365e12]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::deal(donor: [0xBd4D11D884c3c7bF373b013AC52fd99f9DD86D0A], 20000000000000000000 [2e19])
│ └─ ← [Return]
├─ [2383] GivingThanks::tokenCounter() [staticcall]
│ └─ ← [Return] 0
├─ [0] VM::prank(donor: [0xBd4D11D884c3c7bF373b013AC52fd99f9DD86D0A])
│ └─ ← [Return]
├─ [5256] GivingThanks::donate{value: 2365492906797}(charity: [0x59cf1Fbe6AD1EBf1a01e78EE808B7c889E6dd58A])
│ ├─ [0] admin::isVerified(charity: [0x59cf1Fbe6AD1EBf1a01e78EE808B7c889E6dd58A]) [staticcall]
│ │ └─ ← [Stop]
│ └─ ← [Revert] EvmError: Revert
└─ ← [Revert] EvmError: Revert
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.72ms (797.81µs CPU time)
Ran 1 test suite in 4.49ms (1.72ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/GivingThanks.t.sol:GivingThanksTest
[FAIL. Reason: EvmError: Revert; counterexample: calldata=0x06a6cd8900000000000000000000000000000000000000000000000000000226c25d4b2d args=[2365492906797 [2.365e12]]] testFuzzDonate(uint96) (runs: 0, μ: 0, ~: 0)
Encountered a total of 1 failing tests, 0 tests succeeded
POC - 2: More Explicit
This second proof of code comes from a more explicit test, that specific tests what address is being used in the GivingThanks contract for the CharityRegistry data. Please add the following test inside /test/GivingThanks.t.sol:
function test_BrokenInitializedCharityRegistry() public {
CharityRegistry newRegistryContract;
GivingThanks newCharityContract;
vm.prank(admin);
newRegistryContract = new CharityRegistry();
vm.prank(admin);
newCharityContract = new GivingThanks(address(newRegistryContract));
assertNotEq(address(newCharityContract.registry()), address(newRegistryContract));
}
Tools Used
Recommendations
To fix this vulnerability, replace msg.sender with the parameter _registry:
constructor(address _registry) ERC721("DonationReceipt", "DRC") {
- registry = CharityRegistry(msg.sender);
+ registry = CharityRegistry(_registry);
owner = msg.sender;
tokenCounter = 0;
}
