Open code423n4 opened 2 years ago
Dup #16
This contract is designed to be generic. newThreshold < 5
would still allow one operator with weight 5
Good spot. Will consider that
NC
L as the finding is valid and while trust assumptions are not broken, risk is increased (this is equivalent to having a Multisig with one Signer)
NC
1L 2NC
Report
Low
L-01: use two-step process for critical address changes
Consider using a two-step process for transferring the ownership of a contract. While it costs a little more gas, it's safer than transferring directly.
Here's an example from the Compound Timelock contract: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L45-L58
L-02: AxelarAuthWeighted can be configured to only need 1 operator to sign off
The
_transferOperatorship()
function validates certain properties of the new operators. One of them is the check whether the threshold is not 0. I'd argue, that you should increase the minimum limit to a more reasonable number. With the current implementation, the operators can configure the contract so that only 1 operator needs to sign off (threshold == 1
). That could happen by mistake or intentionally. Even then it's most likely not in the best interest of the users that a single entity gains control. The whole idea behind the design is that multiple operators have to sign off.https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L55-L84
Change line 72 to something like:
Non-Critical
N-01: emit an event when changing a contract's configuration
It allows third parties to track those changes efficiently.