Summary
The registry state variable in the GivingThanks contract is initialised with the address of the msg.sender, as opposed to the constructor parameter _registry.
Vulnerability Details
The function GivingThanks::donate relies on the registry::isVerified function to verify that the charity parameter is a whitelisted charity.
function donate(address charity) public payable {
@> require(registry.isVerified(charity), "Charity not verified");
(bool sent,) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
...
}
However, as the registry was initialised with the address of owner as opposed to the correct contract address, the call to the GivingThanks::donate function will revert.
constructor(address _registry) ERC721("DonationReceipt", "DRC") {
@> registry = CharityRegistry(msg.sender);
owner = msg.sender;
tokenCounter = 0;
}
Impact
It is impossible for donors to donate to a charity, as the GivingThanks::donate function will always revert on the first require check.
Proof of Code
Run the following command:
forge test -vvvv --mt testDonate
Output:
Ran 1 test for test/GivingThanks.t.sol:GivingThanksTest
[FAIL. Reason: EvmError: Revert] testDonate() (gas: 27751)
Traces:
[1851260] GivingThanksTest::setUp()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF]
├─ [0] VM::label(admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF], "admin")
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] charity: [0x59cf1Fbe6AD1EBf1a01e78EE808B7c889E6dd58A]
├─ [0] VM::label(charity: [0x59cf1Fbe6AD1EBf1a01e78EE808B7c889E6dd58A], "charity")
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] donor: [0xBd4D11D884c3c7bF373b013AC52fd99f9DD86D0A]
├─ [0] VM::label(donor: [0xBd4D11D884c3c7bF373b013AC52fd99f9DD86D0A], "donor")
│ └─ ← [Return]
├─ [0] VM::prank(admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF])
│ └─ ← [Return]
├─ [191749] → new CharityRegistry@0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395
│ └─ ← [Return] 847 bytes of code
├─ [0] VM::prank(admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF])
│ └─ ← [Return]
├─ [1443803] → new GivingThanks@0x6D9da78B6A5BEdcA287AA5d49613bA36b90c15C4
│ └─ ← [Return] 6753 bytes of code
├─ [0] VM::prank(admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF])
│ └─ ← [Return]
├─ [22464] CharityRegistry::registerCharity(charity: [0x59cf1Fbe6AD1EBf1a01e78EE808B7c889E6dd58A])
│ └─ ← [Stop]
├─ [0] VM::prank(admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF])
│ └─ ← [Return]
├─ [22860] CharityRegistry::verifyCharity(charity: [0x59cf1Fbe6AD1EBf1a01e78EE808B7c889E6dd58A])
│ └─ ← [Stop]
└─ ← [Stop]
[27751] 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
As can be seen, the test failed due to the function call to CharityRegistry::isVerified reverting, as the CharityRegistry contract was initialised with the wrong address.
Tools Used
Manual review & foundry unit tests
Recommended Mitigation
Use the parameter _registry instead of the msg.sender to initialise the CharityRegistry contract for the state variable registry.
constructor(address _registry) ERC721("DonationReceipt", "DRC") {
+ registry = CharityRegistry(_registry);
- registry = CharityRegistry(msg.sender);
owner = msg.sender;
tokenCounter = 0;
}
