Existing minter/operator will not be able to revoke newly proposed addresses

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-axelar-network/blob/4572617124bed39add9025317d2c326acfef29f1/interchain-token-service/contracts/utils/Minter.sol#L40

Vulnerability details

Summary

Existing minters/operators in ITS have the ability to propose mintership or operatorship to another address by calling proposeMintership(). The proposed address can then accept the ownership of the minter/operator role by calling acceptMintership(). This two-step ownership change mechanism has been introduced to mitigate a role from being lost or compromised in the future similar to how OZ's Ownable2Step implementation works.

The issue is that it is not be possible for the current minter/operator to revoke a proposed mintership/operatorship. This would be an issue in the following scenarios:

1. Current minter/operator A proposes the new address B but the decides that the ownership needs to be retained for longer or even forever.
2. Current minter/operator A proposes the new address B but the proposed address B becomes compromised before it accepts the role.
3. Current minter/operator A proposes the new address B but ownership of the role is transferred to B through transferMintership()/transferOperatorship(). In this case, if A is ever transferred the mintership/operatorship back either from B directly or after multiple transfers, B has the power to take the role from A at any time.
4. Current minter/operator A proposes the new address B but ownership of the role is proposed/transferred to another address C due to change of plans or B has been compromised as seen in scenario 2. In this case, if C ever transfers the mintership/operatorship back to A or A receives the role back after multiple transfers, B has the power to take the role from A at any time.
5. Current minter/operator simply supplies an incorrect address.

In addition to scenarios 1,2 and 5, we are particularly interested in scenarios 3 and 4. In these specific scenarios, although the ownership of the role was transferred multiple times, the address B (while being a non-priveleged actor in the system) still retained the power to steal mintership/operatorship from address A. This is a privilege escalation issue since A did not approve/propose B to take the role in the new context i.e. after A regained the ownership of the role either from C (as seen in scenario 4) or B (as seen in scenario 3).

Since Minter.sol and Operator.sol are inherited in ITS, the main contracts that would be affected are:

1. InterchainToken.sol
2. InterchainTokenService.sol
3. TokenManager.sol

Proof of Concept

We'll be looking at Minter.sol here though the same also applies to Operator.sol for the operator role.

1. Current minter A proposes the MINTER role to an address B.

function proposeMintership(address minter_) external onlyRole(uint8(Roles.MINTER)) {
        _proposeRole(msg.sender, minter_, uint8(Roles.MINTER));
    }

2. This would call _proposeRole() inherited from RolesBase.sol, which would further calls _proposeAccountRoles() and _setProposedRoles(), which stores the equivalent of 1 but in binary i.e. 0001 at storage location key. This key represents the proposal from address A to B.

    function _setProposedRoles(
        address fromAccount,
        address toAccount,
        uint256 proposedRoles_
    ) private {
        bytes32 key = _proposalKey(fromAccount, toAccount);
        assembly {
            sstore(key, proposedRoles_)
        }
    }

3. Now if scenarios 1,2 or 5 occur, the current minter would run into the issue of not being able to revoke the proposed mintership to address B due to missing functionality to do so.

4. Let's look at scenario 3. Let's say after proposing address B, address A decides to transfer the role directly to B for reasons such as B takes too long to accept or there needs to be an immediate switch from A to B. In this case, the current minter A calls function transferMintership().

 function transferMintership(address minter_) external onlyRole(uint8(Roles.MINTER)) {
        _transferRole(msg.sender, minter_, uint8(Roles.MINTER));
    }

5. The function transferMintership() calls _transferRole() on the inherited RolesBase.sol contract, which further calls the function _transferAccountRoles(). In this function, the role is removed from address A and added to address B. But the proposed mintership i.e. the proposal key is not cleared from A to B. Due to this, if A ever regains back the mintership either from B directly or after multiple transfers, B retains the ability to steal the role from A.

function _transferAccountRoles(
        address fromAccount,
        address toAccount,
        uint256 accountRoles
    ) internal {
        if (!_hasAllTheRoles(_getRoles(fromAccount), accountRoles)) revert MissingAllRoles(fromAccount, accountRoles);

        _removeAccountRoles(fromAccount, accountRoles);
        _addAccountRoles(toAccount, accountRoles);
    }

6. Lastly let's look at scenario 4. Let's say after proposing address B, address A decides to transfer the role to address C in case there have been a change of plans or simply if B is compromised (as seen in scenario 2). If A ever regains back the mintership either from C directly or after multiple transfers, B retains the ability to steal the role from A due to the proposal key not being cleared from A to B.

Tools Used

Manual Review

Recommended Mitigation Steps

This would require two fixes:

1. Implement a function that allows the current minter to revoke a proposed mintership. For example, minters are proposed with 0001 in binary at storage location key. Consider performing an AND operation with the value stored at key i.e. 0001 or just simply set key to 0.

2. To deal with scenario 3, consider calling _setProposedRoles(fromAccount, toAccount, 0); while the transfer occurs in RolesBase.sol such that any proposed mintership from the "fromAccount" to "toAccount" is cleared.

Assessed type

Governance

[milapsheth]

The report is valid, although the severity is considered Low due to the exploit scenario being exceptional and requires the role to still be proposed to an address that's not trusted later.

[alex-ppg]

The Warden has identified that mintership proposals cannot be revoked and will persist throughout role transfers if the role ever returns to the same account that made the proposals.

While the behavior is indeed non-standard, I do not foresee a medium-risk vulnerability arising from what is described as the persistence of role proposals, which is limited by the fact that the proposals are inherently attached to the proposer. As such, I believe a QA (L) ruling is more appropriate.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-b

[mcgrathcoutinho]

Hey @alex-ppg, thanks for your response! Here is why I believe this issue is of Medium-severity:

1. The team has introduced two-step transfer to avoid roles from being compromised. But one potential scenario was overlooked, which has been described in this report.
2. As we observe in scenarios 3 and 4 described in the report, though the transfer occurs, the proposal would still exist. I believe this is a privilege escalation issue since as mentioned in the report, in the new context, A did not approve/propose B (who is validly a non-privileged actor in the system now).
3. Although as the sponsor mentioned, the likelihood is exceptional, the impact would be high since it would result in a possible take over and full control over either of the three contracts: InterchainToken.sol, InterchainTokenService.sol or TokenManager.sol. For InterchainToken.sol and TokenManager.sol specifically, I believe this issue is not limited to Axelar's own InterchainToken.sol and TokenManager.sol contract instance but to all InterchainToken.sol and TokenManager.sol contract instances that are deployed using InterchainTokenFactory.sol and TokenManagerDeployer.sol to be used as custom token managers, hence further amplifying and exposing the risk to external devs/parties.
4. Due to this exceptional oversight in the code (low likelihood), it could result in control over the core contracts mentioned (high impact). In addition to this, my classification of this submission is tended more towards medium-severity due to the amplified risk this carries when multiple instances of these contracts are deployed by devs using the deployers mentioned in point 3.

Please re-consider this issue, thank you!

[alex-ppg]

Hey @mcgrathcoutinho, thank you for your PJQA contribution!

After reviewing your feedback, I am inclined to retain my original ruling on this submission. The precise exploit scenario involves:

• Honest Actor A being the Minter of the contract
• Honest Actor B being proposed by Actor A, not accepting the proposal at all (this is a low likelihood scenario, as they can acquire ownership and do any malicious actions they wish if they were malicious)
• Honest Actor B receiving ownership from Actor A directly (this is also a low-likelihood scenario, as Actor A would be relinquishing ownership to a potentially inactive actor or an incorrect address)
• Honest Actor B transferring ownership back to Actor A (also a low likelihood event as ownership is usually transferred to new parties each time)
• Honest Actor B turning malicious after transferring ownership back to Actor A, capturing ownership via the elaborate low-likelihood setup earlier

I believe that, by all accounts, the above execution flow (which is the only one this exhibit could have an impact with) is of an extremely low likelihood due to the malicious Actor B having had ownership in the past and thus the opportunity to cause mischief without going through the elaborate vulnerability submitted.

I commend you for identifying the vulnerability as it is important to address, however, I cannot argue that it is of any risk greater than QA (L).

[mcgrathcoutinho]

Hi @alex-ppg, thank you for the feedback on my comment!

One of my main reasoning was that this extremely low likelihood would be amplified when multiple instances of these contracts are deployed by devs/external parties using the deployers. This imo would balance out the extremely low likelihood and turn it into a rather low likelihood scenario + the high impact, thus posing a Medium-risk vulnerability.

Pardon me for commenting after PJQA. I hope this observation on the likelihood and coupling it with the high impact it poses, helps in determining the severity. Thank you!