search

code-423n4 / 2021-06-realitycards-findings

3 stars 2 forks source link

isGovernor excludes Factory owner #75

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

In the Factory contract, the Factory owner is authorized to change approval for governor addresses and is also treated as a governor in the modifier onlyGovernors. However, isGovernor modifier excludes Factory owner as a governor for some reason. This function is used only by Treasury to whitelist users who can deposit tokens and would make sense to include Factory owner as a governor to be consistent.

Without doing so, Factory owner cannot whitelist users without adding itself or someone else as a governor.

Proof of Concept

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCFactory.sol#L360-L365

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCFactory.sol#L181-L188

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L212

Tools Used

Manual Analysis

Recommended Mitigation Steps

Include Factory owner in isGovernor().

Splidge commented 3 years ago

While the owner has the ability to perform the tasks of a governor and the ability to make themselves a governor they might not actually be a governor so probably shouldn't be included in isGovernor as this could be used for other external reasons. The addition of the owner into onlyGovernors is more of a convenience for the owner as they have the power to change this anyway, why make it harder for them?

dmvt commented 3 years ago

I think this issue is a fair critique. The language should probably be cleaned up so future devs don't get confused as to when 'governors' include the factory owner and when not.