VTVL contest - reassor's results

Lines of code

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

Vulnerability details

Description:

Contract VariableSupplyERC20Token is expected to implement ERC20 token logic with limited/unlimited supply specified by the creator. The issue is that the logic of limiting supply does not work properly and will never be enforced because of the faulty logic insidie mint function. Admin can keep minting tokens until mintableSupply reaches 0, but then mint functionality treats the contract as initialized with unlimtied supply and allows further minting.

Scenario:

1. Contract is being initlized with initialSupply_ set to 100 and maxSupply_ set to 0.
2. Admin keeps minting tokens until mintableSupply reaches 0.
3. Admin is still able to mint tokens because contract treats it as it was designed with unlimited supply.

    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;
    }

Proof of Concept

VariableSupplyERC20Token:

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

Recommendation

It is recommended to fix the logic in mint function and check if current supply plus the amount is smaller or equal to max supply. constructor:

(..)
maxSupply = _maxSupply;
(..)

mint:

(..)
require(_totalSupply + amount <= maxSupply);
(..)

Lines of code

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

Vulnerability details

Description

Contract AccessProtected implements functionality for adding and removing administrators and can be used to implement authorization for other contracts. The issue is that function setAdmin allows removing any administrator from the mapping which might lead to accidental loss of administrative control.

Scenario:

1. There is only single admin in the mapping.
2. Admin uses setAdmin and by accident sets own address to false.
3. Administrative control over contracts that use AccessProtected has been lost.

Proof of Concept

AccessProtected.sol:

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

Recommendation

It is recomended to add check to setAdmin function if address admin is not _msgSender():

require(admin != _msgSender());