search

code-423n4 / 2022-08-foundation-findings

0 stars 0 forks source link

Bad access control in AdminRole.sol can lead to griefing DoS by front-running when trying to withdraw treasury funds #258

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/FoundationTreasury.sol#L61

Vulnerability details

Proof of concept

FoundationTreasury.sol inherits CollateralManagement.sol which has the method 

function withdrawFunds(address payable to, uint256 amount) external onlyAdmin

that can withdraw the whole balance of the treasury to the to address. It can be called only by an address that has the admin role, but in AdminRole.sol the revokeAdmin() functionality is external and anyone can call it. So an attacker can scan the mempool for any call to withdrawFunds by an admin and just place a transaction that revokes this admin’s rights with higher gas than the withdraw transaction, which will result in a revert in the withdraw transaction. So a bad actor can just write a script to execute a griefing attack (he will lose gas, but won’t win anything) anytime an admin tries to withdraw.

Impact

This leads to all the protocol treasury funds being stuck in it as there is no other way to get them out.

Recommendation

Revisit the AdminRole.sol smart contract and add access control to revokeAdmin() method. You can make it so that only already set admins (like the one set in _initializeAdminRole()) can revoke other admins.

HardlyDifficult commented 2 years ago

The Treasury contract and it's associated mixins were labeled as out-of-scope.

Also this report is invalid -- see https://github.com/code-423n4/2022-08-foundation-findings/issues/275#issuecomment-1215761575

HickupHH3 commented 2 years ago

Dup of #275