VTVL contest - 0xbepresent's results

Lines of code

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

Vulnerability details

Impact

If maxSupply was assigned different to 0, the mint function should limit the token creation. Comment in the code says:

@param maxSupply_ - What's the maximum supply. The contract won't allow minting over this amount. Set to 0 for no limit.

The problem is that even when the maxSupply was assigned different to 0, the restriction was ignored in the mint function because the function will reduce the mintableSupply until 0 and zero also means "no limit".

An unlimited supply will make the token inflationary, behavior that is not correct if the creator setting up the max supply

Proof of Concept

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

• I made a basic test in Brownie/Python. It helps to figure out the problem. https://gist.github.com/0xbepresent/6b54369211019a3f90d431ccff599fdf

Tools used

VisualStudio/Python

Recommended Mitigation Steps

Consider a standard implementation https://docs.openzeppelin.com/contracts/2.x/erc20-supply#fixed-supply

Lines of code

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

Vulnerability details

Impact

If a wrong/invalid address is given when calling setAdmin, some contract's functions could be lost access. While setAdmin checks for zero address, there is no validation of the new address being correct.

Also, there is no validation that the contract has at least one admin.

The design of the setAdmin function can block the contract's functions forever, so functions like "createClaim", "createClaimsBatch", "withdrawAdmin", "revokeClaim", "withdrawOtherToken", "setAdmin", "mint" would be locked.

contracts/AccessProtected.sol#L39    function setAdmin(address admin, bool isEnabled) public onlyAdmin {

Proof of Concept

• Alice, the admin, executes setAdmin to an address which is accidentally invalid.

setAdmin(invalid_address, True)

• Alice, who is giving up being admin, will invalidate her own access.

setAdmin(alice_address, False)

• Now, nobody has access to critical functions.

Tools used

VisualStudio Code

Recommended Mitigation Steps

Change the single-step admin assignation to a two-step process where the current Admin first approves a new address as a pendingAdmin. That pendingAdmin has to then claim the ownership in a separate transaction. An incorrectly set pendingAdmin can be reset by changing it again to the correct one who can then successfully claim it in the second step.

Also, in the admin assignation process make sure that the contract has at least one admin.

1 - Public functions that are never called from within the contracts should be declared external to save gas

Insatance of this issue:

contracts/VTVLVesting.sol#L398       function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    

2 - Unused imports

Import of unnecessary files costs deployment gas

contracts/VTVLVesting.sol#L6         import "@openzeppelin/contracts/access/Ownable.sol";
contracts/AccessProtected.sol#L5     import "@openzeppelin/contracts/access/Ownable.sol";

3 - Multiple require statements instead of &&

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

contracts/VTVLVesting.sol#L344

require(_startTimestamps.length == length &&
        _endTimestamps.length == length &&
        _cliffReleaseTimestamps.length == length &&
        _releaseIntervalsSecs.length == length &&
        _linearVestAmounts.length == length &&
        _cliffAmounts.length == length, 
        "ARRAY_LENGTH_MISMATCH"
);

4 - Unnecessary checked arithmetic in for loop

There is no risk that the loop counter can overflow, using solidity's unchecked block saves gas.

Instance of this issue:

contracts/VTVLVesting.sol#L353      for (uint256 i = 0; i < length; i++) {

Unchecked implementation example:

for (uint256 i; i < 10;) {
    j++;
    unchecked {
        ++i;
    }
}

5 - x =+ y costs more gas than x = x + y for state variables

Instance of this issue:

contracts/VTVLVesting.sol#L302        numTokensReservedForVesting += allocatedAmount; // track the allocated amount

6 - Amount is not validated

Implement the required statement that amount should be greater than 0 in order to save gas.

Instances of this issue:

contracts/token/VariableSupplyERC20Token.sol#L36             function mint(address account, uint256 amount) public onlyAdmin {