search

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

0 stars 0 forks source link

Bad access control in AdminRole.sol can lead to all funds being stolen from FoundationTreasury.sol #243

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 looks like it is protected because it has the onlyAdmin modifier, which comes from AdminRole.sol. The problem is that the grantAdmin() method in AdminRole.sol is not protected and is external, so anyone can make himself an admin and call withdrawFunds, which results in stolen funds from the treasury.

Impact

The severity is critical and the impact is very serious, because the attack is easy to execute and it can result in 100% of the treasury funds getting stolen.

Recommendation

Revisit the AdminRole.sol smart contract and add access control to grantAdmin() method. You can make it so that only already set admins (like the one set in _initializeAdminRole()) can set new 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