veToken Finance contest - SmartSek's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L77

Vulnerability details

Impact

Compromised owner can withdraw() entire balance of VeTokenMinter.sol to any other account.

Proof of Concept

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L77-L81

function withdraw(address _destination, uint256 _amount) external onlyOwner {
    veToken.safeTransfer(_destination, _amount);

    emit Withdraw(_destination, _amount);
}

The owner can choose any _destination and _amount to send funds to with no delay or limit. These funds could be used to call Booster.deposit() and then Booster.withdraw()(withdraw) the equivalent in lptoken.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing a timelock on VeTokenMinter.withdraw() and changing the destination to an address that owner has no control over.

Example of similar issues illustrating the severity of the finding can be found here (H-09).

QA Report

[L-01] Missing zero address check

VeAssetDepositor.sol#L44-L51

   ) {
        staker = _staker;
        minter = _minter;
        veAsset = _veAsset;
        escrow = _escrow;
        feeManager = msg.sender;
        maxTime = _maxTime;
    }

If any of the immutable variables above is accidentally set to zero the contract would have to be redeployed.

We recommend implementing a zero address check.

Other instances: Booster.sol#L41-L45

[L-02] safeApprove has been deprecated

Booster.sol#L374

We recommend using safeIncreaseAllowance and safeDecreaseAllowance from SafeERC20.

REFERENCE

[N-01] Typo

Verb is not conjugated correctly:

//can locking immediately or defer locking to someone else by paying a fee.

can lock immediately.

VeAssetDepositor.sol#L124

[N-02] Inconsistent handling of booleans

Booster.sol#L350-L353

require(!isShutdown, "shutdown");
        PoolInfo storage pool = poolInfo[_pid];
        require(pool.shutdown == false, "pool is closed");

For better readability please stick with one form of boolean check, either using ! or == false. Please have in mind that == false is slightly more gas expensive.

[N-03] Remove outdated comment

Booster.sol#L233

 //values must be within certain ranges

Comment above suggests variable still are bound to specific ranges, however these ranges have been removed from this implementation. Please remove comment.

[N-04] SafeMath inconsistency

Some calculations redundantly use SafeMath and some dont. Please choose one method (preferably not SafeMath) and stick with it. No SafeMath With SafeMath

[N-05] veTokenMinter.sol doesnt really mint tokens

VeTokenMinter.sol#L72

veTokenMinter.sol doesnt mint tokens. It only really safeTransfer it to another account. Please use a naming convention that is in line with the functionality to speed up the auditing process.

Gas Report

[G-01] Unnecessary use of SafeMath

Underflows/overflows cannot happen in solidity >=0.8.0, therefore SafeMath is redundant.

[G-02] for loop gas optimisation

BaseRewardPool.sol#L199-L201

       for (uint256 i = 0; i < extraRewards.length; i++) {
            IRewards(extraRewards[i]).stake(_for, _amount);
        }

Gas could be saved by:

• Not initialising variable to default value of zero
• Caching array length
• Using a prefix (++i) instead of a postfix (i++)
• Unchecking increment count

Example:

uint length = extraRewards.length;
for (uint256 i; i < length;) {
    IRewards(extraRewards[i]).stake(_for, _amount);
		unchecked { ++i; }
}

[G-03] Unnecessary boolean declaration

Booster.sol#L352

require(pool.shutdown == false, "pool is closed");

pool.shutdown == false can be changed to !pool.shutdown.

[G-04] Zero address check location could be more gas efficient

Booster.sol#L352-L360

        require(pool.shutdown == false, "pool is closed");

        //send to proxy to stake
        address lptoken = pool.lptoken;
        IERC20(lptoken).safeTransferFrom(msg.sender, staker, _amount);

        //stake
        address gauge = pool.gauge;
        require(gauge != address(0), "!gauge setting");

require(gauge != address(0), "!gauge setting"); could be set prior to //send to proxy to stake to save the gas used in safeTransferFrom in case gauge == address(0).

[G-05] Parameters could be immutable

VeTokenMinter.sol#L18

Parameters such as totalCliffs, reductionCliff, veToken and maxTime could be made immutable to save gas.