`makeOwnerAdmin()` is not protecting enough from malicious admin act

[hats-bug-reporter[bot]]

Github username: @chainNue Twitter username: chainNue Submission hash (on-chain): 0xfd21d26e7cda815dea2f3a9bff19d74a4afcbf4282d5cf28d245dac1ec227cbb Severity: medium

Description: Description\

BFX vault have 3 roles, Admin, Trader and Treasurer. The Admin users can add and remove roles, this include admin role of the owner.

File: BfxVault.sol
189:     function addRole(address signer, uint256 role) public {
190:         require(signers[msg.sender][ADMIN_ROLE], "NOT_AN_ADMIN");
191:         signers[signer][role] = true;
192:         emit AddRole(signer, role);
193:     }
...
206:     function removeRole(address signer, uint256 role) public {
207:         require(signers[msg.sender][ADMIN_ROLE], "NOT_AN_ADMIN");
208:         signers[signer][role] = false;
209:         emit RemoveRole(signer, role);
210:     }

Interestingly, there is makeOwnerAdmin() function to restore owner to become admin (again). Meanwhile, owner is immutable (means it will not be changed/transfered), and in constructor this owner already set as admin, so the existance of this makeOwnerAdmin() function raise my assumption, the dev believe there is an edge case possibility a malicious admin can removed the owner as admin.

File: BfxVault.sol
212:     function makeOwnerAdmin() external onlyOwner {
213:         signers[owner][ADMIN_ROLE] = true;
214:     }

But, this (makeOwnerAdmin) backup mechanism doesn't really fixed the situation well.

Again, assuming makeOwnerAdmin() is to restore owner to be admin again due to malicious other admin act, then, this malicious admin need to be cleared or removed by owner.

Since admin can add other roles, it's possible this malicious admin can assign many address to became admin or any roles. Therefore, owner should manually remove this malicious backup admin. It's like chasing each other between owner and malicious admin, which can involve any front-run mechanics.

The only way to 'fix' this is to pause the addRole function, to prevent any further malicious role being added.

Attack Scenario\

1. Alice is owner (and admin), Bob, Carol are assigned by Alice as admin
2. Bob turns out act maliciously, and remove Alice, and Carol as Admin
3. Alice gain her admin via makeOwnerAdmin()
4. Bob sees Alice gain her admin, thus he now trying to assign other backup address as Admin
5. Alice keep up head to head with Bob, by removing role of Bob's backup address
6. Bob keep front-run Alice by adding more address as Admin, meanwhile he can act maliciously by gaining the Admin role.

Even this issue likelyhood is Low, but the impact and severity is high. Therefore, I assign this a medium one.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation

Consider to add pausable mechanism on addRole which is accessible only by owner

[alex-sumner]

The scenario first requires that a malicious actor is made an admin, and then that they devote themselves to keeping their position by continually creating new backup admins. Even if they do this the severity is not high, they cannot access funds or prevent operation of the vault.

[chainNue]

Hello, @alex-sumner, yes I'm totally agree with you, this is not a High severity, that's why I'm setting this severity issue as a Medium, please kindly check the severity status of this issue I submitted.

This issue state the reasoning behind makeOwnerAdmin() and when malicious admin exist, it will keep being in the system, unable to be removed, and also they can have access to TRADER_ROLE, as comments says, trader will have ability to trade on the bfx exchange with the vault's funds, which they can maliciously do a bad trade or unreasonable trade (even though the implementation is oos). Could you please reconsider this issue again? thanks.

File: BfxVault.sol
093:     /**
094:      * @notice does the user have the TRADER_ROLE - which gives 
095:      * the ability to trade on the bfx exchange with the vault's funds
096:      * 
097:      * @param user the address to check
098:      * @return true if the user has the TRADER_ROLE 
099:      */
100:     function isTrader(address user) public view returns (bool) {
101:         return signers[user][TRADER_ROLE];
102:     }

[alex-sumner]

This is not a medium severity issue. Any actor given a privileged role, such as admin or trader, must be trusted. Giving such a role to a bad actor does allow them to act maliciously in ways a less trusted user could not. However, it will not allow them to withdraw funds in the scenario you describe and adding a pause function is not necessary as they can simply be listed as a malicious account by the exchange, then they can no longer trade.

[chainNue]

Surely there is a true reason makeOwnerAdmin() function exist.

• if owner was removed as admin by other admin by mistake, then other admin can assign the owner as admin again by the remover.

if not owner being removed by other malicious admin, I don't see any other possibilities. Maybe if you explain your case, I will understand well? IMO, the only logical reason the makeOwnerAdmin() exist is for failsafe mechanism if there is a malicious admin removing all other admin.

if dev believe admins will 100% behave correctly, then there is no need to have this makeOwnerAdmin function.

---

I never mention any fund withdrawal in my issue, I only mention trader role has ability to trade vault's funds, which is written on isTrader function NatSpec.

Adding pause is just one example to prevent malicious admin do malicious role add/remove act.

..as they can simply be listed as a malicious account by the exchange, then they can no longer trade.

and then, the malicious admin will create and assign another account, and keep doing that, so this is an issue.

[alex-sumner]

The reason for the makeOwnerAdmin() function was that an admin can remove themselves as an admin, potentially leaving the contract with no admins. This function allows recovery from that state.