VTVL contest - karanctf's results

Low

[L-1] Open TODOs

VTVLVesting.sol:266:        // Potential TODO: sanity check, if _linearVestAmount == 0, should we perhaps force that start and end ts are the same?

[L-2] Missing checks for address(0x0) when assigning values to address state variables filter for non address vairable

VTVLVesting.sol:83:        tokenAddress = _tokenAddress;

Non Critical

[N-1] Try making public function internal if there is no use in other contracts like division multiply round etc

VTVLVesting.sol:206:    function finalVestedAmount(address _recipient) public view returns (uint112) {
VTVLVesting.sol:398:    function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    

[N-2] Use scientific notation (e.g. 10e18) rather than exponentiation (e.g. 10**18)

token/FullPremintERC20Token.sol:9:    // uint constant _initialSupply = 100 * (10**18);
test/TestERC20Token.sol:7:    // uint constant _initialSupply = 100 * (10**18);
test/TestERC20Token.sol:9:        uint256 supplyToMint = initialSupply_ * (10 ** decimals());

[G-1] Use preincrement to save gas

VTVLVesting.sol:353:        for (uint256 i = 0; i < length; i++) {

[G-2] Splitting require() statements that use && saves gas

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement:

VTVLVesting.sol:343:        uint256 length = _recipients.length;
VTVLVesting.sol:344:        require(_startTimestamps.length == length &&
VTVLVesting.sol:345:                _endTimestamps.length == length &&
VTVLVesting.sol:346:                _cliffReleaseTimestamps.length == length &&
VTVLVesting.sol:347:                _releaseIntervalsSecs.length == length &&
VTVLVesting.sol:348:                _linearVestAmounts.length == length &&
VTVLVesting.sol:349:                _cliffAmounts.length == length, 

[G-3] Donot use default values Explicit initialization with zero is not required for variable declaration of numTokens because uints are 0 by default.removeing this will reduce contract size and save a bit of gas.

VTVLVesting.sol:148:        uint112 vestAmt = 0;
VTVLVesting.sol:353:        for (uint256 i = 0; i < length; i++) {

[G-4] public functions not called by the contract should be declared external instead

VTVLVesting.sol:398:    function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    

[G-5] variable == false|0 -> !variable or variable == true -> variable

VTVLVesting.sol:111:        require(_claim.isActive == true, "NO_ACTIVE_CLAIM");
VTVLVesting.sol:129:        require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
VTVLVesting.sol:264:        require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
VTVLVesting.sol:276:                _cliffReleaseTimestamp == 0 && 
VTVLVesting.sol:277:                _cliffAmount == 0

[G-06] Use != 0 instead of > 0 in HolyPaladinToken.sol

token/VariableSupplyERC20Token.sol:27:        require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
token/VariableSupplyERC20Token.sol:31:        if(initialSupply_ > 0) {
token/VariableSupplyERC20Token.sol:40:        if(mintableSupply > 0) {
token/FullPremintERC20Token.sol:11:        require(supply_ > 0, "NO_ZERO_MINT");
VTVLVesting.sol:107:        require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");
VTVLVesting.sol:256:        require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
VTVLVesting.sol:257:        require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
VTVLVesting.sol:263:        require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
VTVLVesting.sol:272:                _cliffReleaseTimestamp > 0 && 
VTVLVesting.sol:273:                _cliffAmount > 0 && 
VTVLVesting.sol:449:        require(bal > 0, "INSUFFICIENT_BALANCE");

[G-7] <x> += <y>costs more gas than<x> = <x> + <y>`

token/VariableSupplyERC20Token.sol:43:            mintableSupply -= amount;
VTVLVesting.sol:161:                vestAmt += _claim.cliffAmount;
VTVLVesting.sol:179:                vestAmt += linearVestAmount;
VTVLVesting.sol:301:        numTokensReservedForVesting += allocatedAmount; // track the allocated amount
VTVLVesting.sol:381:        usrClaim.amountWithdrawn += amountRemaining;
VTVLVesting.sol:383:        numTokensReservedForVesting -= amountRemaining;
VTVLVesting.sol:433:        numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

[G-8] Add a check if new admin is already a admin or the one to be removed is already

removed or non exist this will save gas by avoiding rewrite of same data

AccessProtected.sol:39: function setAdmin(address admin, bool isEnabled) public onlyAdmin {
AccessProtected.sol:40:         require(admin != address(0), "INVALID_ADDRESS");
AccessProtected.sol:41:         _admins[admin] = isEnabled;
AccessProtected.sol:42:         emit AdminAccessSet(admin, isEnabled);
AccessProtected.sol:43:     }