The supply functions of the StablecoinHandler should be marked as internal as it allows the swapper to mint and burn invalid tokens directly

Summary

The swapper could potentially mint and burn invalid tokens through the public functions of StablecoinHandler which are exposed through the AmirX contract as they are.

Vulnerability Detail

The public functions, namely swapAndSend, convertToEXYZ, and convertFromEXYZ, which are defined in the StablecoinHandler contract don't check for the validity of the origin and target tokens. Hence, they should be marked as internal.

Impact

The swapper could potentially mint and burn even those tokens that were explicitly marked as invalid by the maintainer. Moreover, those tokens could also potentially be passed to these functions that were not explicitly marked as invalid, as the default validity for every token (or every address) remains false

Code Snippet

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/21920190e0772afa18e7f856a036fea3ef5b9635/telcoin-contracts/contracts/stablecoin/StablecoinHandler.sol#L116

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/21920190e0772afa18e7f856a036fea3ef5b9635/telcoin-contracts/contracts/stablecoin/StablecoinHandler.sol#L141

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/21920190e0772afa18e7f856a036fea3ef5b9635/telcoin-contracts/contracts/stablecoin/StablecoinHandler.sol#L166

PoC

This is the entire Hardhat test file that includes the tests to demonstrate the issue

```js import { SignerWithAddress } from "@nomicfoundation/hardhat-ethers/signers"; import { expect } from "chai"; import { ethers } from "hardhat"; import { Stablecoin, AmirX, TestToken } from "../../typechain-types"; describe("StablecoinHandler", () => { const MINTER_ROLE = ethers.keccak256(ethers.toUtf8Bytes('MINTER_ROLE')); const BURNER_ROLE = ethers.keccak256(ethers.toUtf8Bytes('BURNER_ROLE')); const SWAPPER_ROLE = ethers.keccak256(ethers.toUtf8Bytes('SWAPPER_ROLE')); const MAINTAINER_ROLE = ethers.keccak256(ethers.toUtf8Bytes('MAINTAINER_ROLE')); let deployer: SignerWithAddress; let holder: SignerWithAddress; let eUSD: Stablecoin; let eMXN: Stablecoin; let USDC: TestToken; let stablecoinHandler: AmirX; beforeEach("setup", async () => { [deployer, holder] = await ethers.getSigners(); const StablecoinHandler_Factory = await ethers.getContractFactory("AmirX", deployer); stablecoinHandler = await StablecoinHandler_Factory.deploy(); }); describe("Alter supply", () => { beforeEach("setup", async () => { const Stablecoin_Factory = await ethers.getContractFactory("Stablecoin", deployer); eUSD = await Stablecoin_Factory.deploy(); eMXN = await Stablecoin_Factory.deploy(); await eUSD.initialize("US Dollar", "eXYZ", 6); await eUSD.grantRole(MINTER_ROLE, stablecoinHandler); await eUSD.grantRole(BURNER_ROLE, stablecoinHandler); await eMXN.initialize("Mexican Peso", "eMXN", 6); await eMXN.grantRole(MINTER_ROLE, stablecoinHandler); await eMXN.grantRole(BURNER_ROLE, stablecoinHandler); await stablecoinHandler.initialize(); await stablecoinHandler.grantRole(MAINTAINER_ROLE, deployer); await stablecoinHandler.grantRole(SWAPPER_ROLE, deployer); }); describe("invalid coins are also swapped", () => { beforeEach("setup", async () => { await stablecoinHandler.grantRole(SWAPPER_ROLE, deployer); // following coins are marked as invalid by passing `false` await stablecoinHandler.UpdateXYZ(eUSD, false, 1000000000, 0); await stablecoinHandler.UpdateXYZ(eMXN, false, 1000000000, 0); await eUSD.grantRole(MINTER_ROLE, deployer); const Token_Factory = await ethers.getContractFactory("TestToken", deployer); USDC = await Token_Factory.deploy("US Dollar Coin", "USDC", 6, holder.address, 10); }); it("swapAndSend", async () => { await eUSD.mintTo(holder, 10); const inputs = { destination: holder, origin: eUSD, oAmount: 10, target: eMXN, tAmount: 100 } await eUSD.connect(holder).approve(stablecoinHandler, 10); await expect(stablecoinHandler.swapAndSend(holder, inputs)).to.not.be.reverted; expect(await eUSD.totalSupply()).to.equal(0); expect(await eMXN.balanceOf(holder)).to.equal(100); }); it("convertToEXYZ", async () => { const inputs = { destination: holder, origin: USDC, oAmount: 10, target: eMXN, tAmount: 100 } await USDC.connect(holder).approve(stablecoinHandler, 10); await expect(stablecoinHandler.convertToEXYZ(holder, deployer, inputs)).to.not.be.reverted; expect(await USDC.balanceOf(deployer)).to.equal(10); expect(await eMXN.balanceOf(holder)).to.equal(100); }); it("convertFromEXYZ", async () => { await USDC.mintTo(deployer, 10); await USDC.connect(deployer).approve(stablecoinHandler, 10); const inputs = { destination: holder, origin: eUSD, oAmount: 10, target: USDC, tAmount: 10 } await eUSD.mintTo(holder, 10); await eUSD.connect(holder).approve(stablecoinHandler, 10); await expect(stablecoinHandler.convertFromEXYZ(holder, deployer, inputs)).to.not.be.reverted; expect(await eUSD.totalSupply()).to.equal(0); expect(await USDC.balanceOf(deployer)).to.equal(0); }); }); }); }); ```

Tool used

Manual Review

Recommendation

Change the visibility of the swapAndSend, convertToEXYZ, and convertFromEXYZ functions to internal instead of public.

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

sweetjimmy - The supply functions of the StablecoinHandler should be marked as internal as it allows the swapper to mint and burn invalid tokens directly #83