A blacklisted address can still hold privileged role such as MINTER, BURNER, SUPPORT, and BLACKLISTER, and still act maliciously

Summary

The protocol has privileged roles: MINTER, BURNER, SUPPORT, and BLACKLISTER. An address can have these roles, and get blacklisted, and yet retain these roles. This gets more interesting when the user being blacklisted is the blacklister, as then the user can remove themselves from being blacklisted. Also, if the blacklisted user still retains the minter/burner role, then they can still act maliciously, which is something the protocol tried to mitigate using the blacklisted role.

Links to affected code

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/util/abstract/Blacklist.sol#L101

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/util/abstract/Blacklist.sol#L32

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/stablecoin/Stablecoin.sol#L123

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/stablecoin/Stablecoin.sol#L31-L33

Vulnerability Detail

The Stablecoin contract has the following behaviours defined:

Do you expect to use any of the following tokens with non-standard behaviour with the smart contracts?

Blacklisting on the stablecoin contract.

Furthermore, the Stablecoin README says:

## Contracts

### `Stablecoin`

.......built-in mechanism for blacklisting addresses to prevent misuse.

#### Key Stablecoin Features

...
...

Blacklisting functionality to restrict transactions from specific addresses.

...
...

The contract further defines these roles: MINTER, BURNER, SUPPORT, and BLACKLISTER. The roles are used to interact with the access controlled functions:

mint() - MINTER
mintTo() - MINTER
burn() - BURNER
burnFrom() - BURNER
erc20Rescue() - SUPPORT
addBlackList() - BLACKLISTER
removeBlackList() - BLACKLISTER

However, when the address is blacklisted, the roles are not cleared for the address being blacklisted.

Impact

If the address that is being blacklisted has one of the privileged roles, then the blacklisted user can still act maliciously and exploit the privileged role.

If the address had BLACKLISTER role, then they can even remove themselves (and others) from the blacklist, and rather blacklist everyone else.

Code Snippet

Place the below code in test/stablecoins/Stablecoin.test.ts:

describe("Privileged Roles of Blacklisted User", () => {
    let blacklistedMinter: SignerWithAddress;
    let blacklistedBurner: SignerWithAddress;
    let blacklistedSupport: SignerWithAddress;
    let blacklistedBlacklister: SignerWithAddress;

    beforeEach("Setup", async () => {
        [
            deployer,
            blacklistedMinter,
            blacklistedBurner,
            blacklistedSupport,
            blacklistedBlacklister,
        ] = await ethers.getSigners();

        await stablecoin.grantRole(BLACKLISTER_ROLE, deployer);

        await stablecoin.grantRole(MINTER_ROLE, blacklistedMinter);
        await stablecoin.grantRole(BURNER_ROLE, blacklistedBurner);
        await stablecoin.grantRole(SUPPORT_ROLE, blacklistedSupport);
        await stablecoin.grantRole(BLACKLISTER_ROLE, blacklistedBlacklister);
    });

    it("Blacklisted users retain privileged role", async () => {
        // Correct role setup
        expect(await stablecoin.hasRole(MINTER_ROLE, blacklistedMinter)).to.equal(true);
        expect(await stablecoin.hasRole(BURNER_ROLE, blacklistedBurner)).to.equal(true);
        expect(await stablecoin.hasRole(SUPPORT_ROLE, blacklistedSupport)).to.equal(true);
        expect(await stablecoin.hasRole(BLACKLISTER_ROLE, blacklistedBlacklister)).to.equal(true);

        // Users get blacklisted
        await stablecoin.connect(deployer).addBlackList(blacklistedMinter);
        await stablecoin.connect(deployer).addBlackList(blacklistedBurner);
        await stablecoin.connect(deployer).addBlackList(blacklistedSupport);
        await stablecoin.connect(deployer).addBlackList(blacklistedBlacklister);

        // Verify Blacklisting
        expect(await stablecoin.blacklisted(blacklistedMinter)).to.equal(true);
        expect(await stablecoin.blacklisted(blacklistedBurner)).to.equal(true);
        expect(await stablecoin.blacklisted(blacklistedSupport)).to.equal(true);
        expect(await stablecoin.blacklisted(blacklistedBlacklister)).to.equal(true);

        // Users retain their roles
        expect(await stablecoin.hasRole(MINTER_ROLE, blacklistedMinter)).to.equal(true);
        expect(await stablecoin.hasRole(BURNER_ROLE, blacklistedBurner)).to.equal(true);
        expect(await stablecoin.hasRole(SUPPORT_ROLE, blacklistedSupport)).to.equal(true);
        expect(await stablecoin.hasRole(BLACKLISTER_ROLE, blacklistedBlacklister)).to.equal(true);

        // Blacklisted MINTER_ROLE can still mint tokens
        await stablecoin.connect(blacklistedMinter).mintTo(blacklistedBurner, 100);
        expect(await stablecoin.balanceOf(blacklistedBurner)).to.equal(100);

        // Blacklisted BURNER_ROLE can still burn tokens
        await stablecoin.connect(blacklistedBurner).burn(100);
        expect(await stablecoin.balanceOf(blacklistedBurner)).to.equal(0);

        // Blacklisted SUPPORT_ROLE can still rescue tokens
        await stablecoin.connect(blacklistedMinter).mintTo(stablecoin, 100);
        expect(await stablecoin.balanceOf(stablecoin)).to.equal(100);
        await stablecoin.connect(blacklistedSupport).erc20Rescue(stablecoin, blacklistedSupport, 100);
        expect(await stablecoin.balanceOf(blacklistedSupport)).to.equal(100);

        // Blacklisted BLACKLISTER_ROLE can still remove address from blacklist, including self
        await stablecoin.connect(blacklistedBlacklister).removeBlackList(blacklistedMinter);
        await stablecoin.connect(blacklistedBlacklister).removeBlackList(blacklistedBurner);
        await stablecoin.connect(blacklistedBlacklister).removeBlackList(blacklistedSupport);
        await stablecoin.connect(blacklistedBlacklister).removeBlackList(blacklistedBlacklister);

        // Verify removed from Blacklist
        expect(await stablecoin.blacklisted(blacklistedMinter)).to.equal(false);
        expect(await stablecoin.blacklisted(blacklistedBurner)).to.equal(false);
        expect(await stablecoin.blacklisted(blacklistedSupport)).to.equal(false);
        expect(await stablecoin.blacklisted(blacklistedBlacklister)).to.equal(false);
    });
});

Tool used

Manual Review

Recommendation

When adding a user to the blacklist, take away all the privileged role that have been granted to the user as well.

// contracts/stablecoin/Stablecoin.sol

function _onceBlacklisted(address user) internal override {
    _transfer(user, _msgSender(), balanceOf(user));
+   _revokeRole(MINTER_ROLE, user);
+   _revokeRole(BURNER_ROLE, user);
+   _revokeRole(SUPPORT_ROLE, user);
+   _revokeRole(BLACKLISTER_ROLE, user);
}

sherlock-audit / 2024-02-telcoin-platform-audit-update-judging

neocrao - A blacklisted address can still hold privileged role such as MINTER, BURNER, SUPPORT, and BLACKLISTER, and still act maliciously #59