VTVL contest - innertia's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L28 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L36

Vulnerability details

VariableSupplyERC20Token can initially set the value of maxSupply to mintableSupply.
Suppose you initially set the maxSupply to 1000. Indeed, due to the constraints of the mint function, we cannot mint more than 1000 at a time. Each minting under 1000 will decrease mintableSupply, and finally, after 1000 mints, it will be zero.
Here is the problem: mintableSupply = 0 means there is no minting limit.
In other words, even though maxSupply was initially set, you can mint an unlimited number of mints even after maxSupply has been minted.
This makes maxSupply meaningless.

Proof of Concept

mintableSupply = maxSupply_;

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L28

function mint(address account, uint256 amount) public onlyAdmin {
        require(account != address(0), "INVALID_ADDRESS");
        // If we're using maxSupply, we need to make sure we respect it
        // mintableSupply = 0 means mint at will
        if(mintableSupply > 0) {
            require(amount <= mintableSupply, "INVALID_AMOUNT");
            // We need to reduce the amount only if we're using the limit, if not just leave it be
            mintableSupply -= amount;
        }
        _mint(account, amount);
    }

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L36

Recommended Mitigation Steps

There is a way to set the initial value of mintableSupply to the maximum value of uint256 and not reset it when maxSupply_= 0.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L39

Vulnerability details

When removing admins' privileges, you can set your own address. This in itself is not a problem, but if you do this while there is only one Admin, there will be zero Admins.
After that, no one will be able to perform operations that require admin privileges.

Proof of Concept

function setAdmin(address admin, bool isEnabled) public onlyAdmin {
        require(admin != address(0), "INVALID_ADDRESS");
        _admins[admin] = isEnabled;
        emit AdminAccessSet(admin, isEnabled);
    }

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L39

Recommended Mitigation Steps

• Keep a count of the number of admins and keep it from going to zero.
• Make it impossible to disenfranchise one's own address.