Axelar Network v2 contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L55-L84

Vulnerability details

Impact

Following scenario: There are 7 operators and the necessary threshold to transfer operatorship is 3.

3 of the 7 operators turn rogue and want to kick out the rest of them. They write a smart contract that calls transferOperatorship() 16 times (see AxelarAuthWeighted.OLD_KEY_RETENTION) with the final call being the desired operator state. They run the contract's function and successfully lock out the rest of the operators without them being able to do anything. All the 16 operator states that are usable are from the attackers. Any subsequent call by honest operators to AxelarAuth.validateProof() will fail.

A more reasonable approach would be to allow the transferOperatorship() function to be only called once in a given timeframe. That way, no operator can loose their access in a matter of a single transaction. This would give them time to react. Additionally, there should be a mechanism for an older set of operators to gain back control by being allowed to call transferOperatorship() as well. Currently, the current operators are the only ones who can transfer operatorship. This means, that you can't block a rogue set of operators from taking over control over transfer mechanism.

Currently, there seems to be a trust that operators can always be trusted. You can't be sure that it will be always the case. Equipping your contract to handle such a situation might come handy at some point.

Proof of Concept

https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L55-L84

Tools Used

none

Recommended Mitigation Steps

Add a state variable that keeps track of the last time operators were changed. Only allow operators to be changed after a day has passed. Also, allow past operators within the retention period to be able to transfer operatorship as well.

Report

• Low
  • L-01: use two-step process for critical address changes
  • L-02: AxelarAuthWeighted can be configured to only need 1 operator to sign off
• Non-Critical
  • N-01: emit an event when changing a contract's configuration

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

• https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/Ownable.sol#L21
• https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/util/Proxy.sol#L26 (Owner part)

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:

if (newThreshold < 5 || totalWeight < newThreshold) revert InvalidThreshold();

Non-Critical

N-01: emit an event when changing a contract's configuration

It allows third parties to track those changes efficiently.

• https://github.com/code-423n4/2022-07-axelar/blob/main/xc20/contracts/XC20Wrapper.sol#L48
• https://github.com/code-423n4/2022-07-axelar/blob/main/xc20/contracts/XC20Wrapper.sol#L45
• https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/util/Proxy.sol#L26

Gas Report

• G-01: use unchecked when incrementing for loop variable
• G-02: increment using ++i instead of i++ consistently
• G-03: use solmate's ERC20 contract
• G-04: remove unnecessary require statment in XC20Wrapper.sol

G-01: use unchecked when incrementing for loop variable

Since the loop's iterator can't realistically overflow because of the loop's condition, you can increment it in an unchecked block to save gas:

./contracts/auth/AxelarAuthWeighted.sol:17:        for (uint256 i; i < recentOperators.length; ++i) {
./contracts/auth/AxelarAuthWeighted.sol:69:        for (uint256 i = 0; i < weightsLength; ++i) {
./contracts/auth/AxelarAuthWeighted.sol:98:        for (uint256 i = 0; i < signatures.length; ++i) {
./contracts/auth/AxelarAuthWeighted.sol:116:        for (uint256 i; i < accounts.length - 1; ++i) {
./contracts/gas-service/AxelarGasService.sol:123:        for (uint256 i; i < tokens.length; i++) {
./contracts/deposit-service/AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
./contracts/deposit-service/AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
./contracts/deposit-service/AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {
./contracts/AdminMultisigBase.sol:51:        for (uint256 i; i < adminCount; ++i) {
./contracts/AdminMultisigBase.sol:158:        for (uint256 i; i < adminLength; ++i) {
./contracts/AxelarGateway.sol:195:        for (uint256 i; i < adminCount; ++i) {
./contracts/AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {
./contracts/AxelarGateway.sol:293:        for (uint256 i; i < commandsLength; ++i) {
./contracts/auth/AxelarAuthWeighted.sol:101:            for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {}

Just search for "for (" in your .sol files to find all the relevant instances

G-02: increment using ++i instead of i++ consistently

Using ++i saves gas because you don't have to cache the original value in memory. You already use it in some places but not consistently.

./contracts/gas-service/AxelarGasService.sol:123:        for (uint256 i; i < tokens.length; i++) {
./contracts/deposit-service/AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
./contracts/deposit-service/AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
./contracts/deposit-service/AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {
./contracts/AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {

G-03: use solmate's ERC20 contract

solmate's contracts are optimized for gas usage. They are more efficient than OZ's contracts. Consider switching to them. It will save gas for all the ERC20 transfers, burns, etc.

https://github.com/transmissions11/solmate

G-04: remove unnecessary require statement in XC20Wrapper.sol

In XC20Wrapper.unwrap() the function checks whether the caller has enough tokens to burn before calling the burn() function. But, the burn function will simply revert if the user doesn't have enough. Explicitly checking for that is a waste of gas.

https://github.com/code-423n4/2022-07-axelar/blob/main/xc20/contracts/XC20Wrapper.sol#L85