hats-finance / ether-fi-0x36c3b77853dec9c4a237a692623293223d4b9bc4

Smart Contracts for Ether Fi dapp
1 stars 1 forks source link

Once an address is made an admin via updateAdmin, there is no way to revoke that privilege #4

Open hats-bug-reporter[bot] opened 11 months ago

hats-bug-reporter[bot] commented 11 months ago

Github username: -- Submission hash (on-chain): 0x26361656404c6c1b17e5a2cee60e3e4f891ec6caa7061603989d60fbd69dbffa Severity: medium

Description: Description\ The admins mapping is defined as: mapping(address => bool) public admins; This mapping stores admin status – true means the address is an admin, false means they are not. The updateAdmin function only allows setting the value to true: function updateAdmin(address _address, bool _isAdmin) external onlyOwner { admins[_address] = _isAdmin; } There is no functionality to ever set an address back to false once made an admin. This means any address that is ever set as an admin via updateAdmin will remain an admin forever. There is no way to revoke their privileges.

Attack Scenario\ Once an address is made an admin via updateAdmin, there is no way to revoke that privilege

Attachments https://github.com/GadzeFinance/dappContracts/blob/68bf2597086d9aa39968c504f04cf34aa0f864c0/src/EtherFiNodesManager.sol#L415-L417

Recommendation\

Add this to the code:

function revokeAdmin(address _address) external onlyOwner { admins[_address] = false; }

ololade97 commented 11 months ago

Here is the correct code attachment:

https://github.com/hats-finance/ether-fi-0x36c3b77853dec9c4a237a692623293223d4b9bc4/blob/180c708dc7cb3214d68ea9726f1999f67c3551c9/src/EtherFiNodesManager.sol#L438-L443

seongyun-ko commented 11 months ago

updateAdmin(addr, false) will do