Tigris Trade contest - Madalad's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L19 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L124-L165 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L189-L204

Vulnerability details

Impact

In GovNFT, the state variable maxBridge is not used anywhere in the contract except it's setter function. This means it is either unneeded, or some intended contract functionality is missing.

As far as I can tell, this variable is meant to limit the amount of NFTs that can be bridged in a single transaction. This is likely so that _nonblockingLzReceive does not revert when the uint array tokenId is too large, as it contains a for loop iterating over the list and would risk running out of gas.

If this functionality is intended, extra require statements should be added to crossChain and/or _nonblockingLzReceive to ensure the amount of NFTs trying to be bridged is below maxBridge. Otherwise, the state variable and setter function should be removed to save gas.

Recommended Mitigation Steps

Either remove the maxBridge variable, or update one/both of the following functions accordingly:

function crossChain(...) public payable {
    require(tokenId.length > 0, "Not bridging");
    require(tokenId.length < maxBridge, "Too many"); // new line

    //...
}

function _nonblockingLzReceive(...) internal {
    
    //...

    require(tokenId.length < maxBridge, "Too many"); // new line

    // mint the tokens
    for (uint i=0; i<tokenId.length; i++) {
        _bridgeMint(toAddress, tokenId[i]);
    }
    emit ReceiveNFT(_srcChainId, toAddress, tokenId);
}

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.