Vader Protocol contest - pants's results

Handle

pants

Vulnerability details

Vulnerability details

These files has open TODOs:

• VaderRouter.sol
• VaderPool.sol
• VaderBond.sol
• USDV.sol

Impact

Open TODOs can hint at programming or architectural errors that still need to be fixed.

Tool Used

Manual code review.

Recommended Mitigation Steps

Resolve these TODOs and bubble up the errors.

Handle

pants

Vulnerability details

You initialize the index variable of the for loop to 0 although it's already set to 0 when declared. appears at GovernorAlpha.sol line 571 and LinearVesting.sol line 72.

for (uint256 i = 0; i < _targets.length; i++) {
    ...
}

Can be changed to (to save gas):

for (uint256 i; i < _targets.length; i++) {
    ...
}

Handle

pants

Vulnerability details

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. https://github.com/code-423n4/2021-09-sushimiso-findings/issues/134

For example "BasePool::swap: Unfavourable Trade" can be changed to "BasePool swap Unfavourable Trade". (BasePool.sol line 324)

Handle

pants

Vulnerability details

You can cache the array length to save length access gas cost at every iteration. The following (bad) form appears at GovernorAlpha.sol line 571 and LinearVesting.sol line 72. GovernorAlpha.sol example:

        for (uint256 i = 0; i < _targets.length; i++) {
            if (_targets[i] == address(this)) {
                revert(
                    "GovernorAlpha::veto: council cannot veto on proposal having action with address(this) as target"
                );
            }
        }

Change it to:

        uint256 len = _targets.length;
        for (uint256 i = 0; i < len; i++) {
            if (_targets[i] == address(this)) {
                revert(
                    "GovernorAlpha::veto: council cannot veto on proposal having action with address(this) as target"
                );
            }
        }

Handle

pants

Vulnerability details

These functions use postfix increaments (i++) instead of uncheked postfix increaments (unchecked{i++}): For example GovernorAlpha.sol line 571 and LinearVesting.sol line 72.

The index is uint256 therefore cannot overflow (gas limit).

Handle

pants

Vulnerability details

Vulnerability details

These functions use postfix increaments (x++) instead of prefix increaments (++x): For example GovernorAlpha.sol line 571 and LinearVesting.sol line 72.

Impact

Prefix increaments are cheaper than postfix increaments.

Tool Used

Manual code review.

Recommended Mitigation Steps

Change all postfix increaments to prefix increaments where it doesn't affects the functionality.

Handle

pants

Vulnerability details

Vulnerability details

These public functions are never called by their contract (a very partial list):

• GovernorAlpha.propose
• GovernorAlpha.execute
• GovernorAlpha.getActions
• GovernorAlpha.getReceipt

Therefore, their visibility can be reduced to external.

Impact

external functions are cheaper than public functions.

Proof of Concept

https://gus-tavo-guim.medium.com/public-vs-external-functions-in-solidity-b46bcf0ba3ac

Tool Used

Manual code review.

Recommended Mitigation Steps

Define these functions as external.

Handle

pants

Vulnerability details

unessesary safe math in UniSwapV2Pair.sol line 120.

if (rootK > rootKLast) {
    uint256 numerator = totalSupply * (rootK - rootKLast);
    uint256 denominator = (rootK * 5) + rootKLast;
    uint256 liquidity = numerator / denominator;
    if (liquidity > 0) _mint(feeTo, liquidity);
}

Here rootK - rootKLast could be calculated with unchecked. As mentioned in the main git page, UniSwapV2Pair.sol is a part of the contest.

if (rootK > rootKLast) {
    uint256 numerator = totalSupply * (unchecked{rootK - rootKLast});
    uint256 denominator = (rootK * 5) + rootKLast;
    uint256 liquidity = numerator / denominator;
    if (liquidity > 0) _mint(feeTo, liquidity);
}

Handle

pants

Vulnerability details

use calldata instead of memory when possible to save GAS

For example in LinearVesting.sol the following function (constructor) could be changed to:

constructor(
    IERC20 _vader,
    address[] memory vesters,
    uint192[] memory amounts
) {
    require(
        _vader != IERC20(_ZERO_ADDRESS) && vesters.length == amounts.length,
        "LinearVesting::constructor: Misconfiguration"
    );

    vader = _vader;

    uint256 total;
    for (uint256 i = 0; i < vesters.length; i++) {
        require(
            amounts[i] != 0,
            "LinearVesting::constructor: Incorrect Amount Specified"
        );
        vest[vesters[i]].amount = amounts[i];
        total = total + amounts[i];
    }
    require(
        total == _TEAM_ALLOCATION,
        "LinearVesting::constructor: Invalid Vest Amounts Specified"
    );

    transferOwnership(address(_vader));
}

could be changed to:

constructor(
    IERC20 _vader,
    address[] calldata vesters,
    uint192[] calldata amounts
) {
    require(
        _vader != IERC20(_ZERO_ADDRESS) && vesters.length == amounts.length,
        "LinearVesting::constructor: Misconfiguration"
    );

    vader = _vader;

    uint256 total;
    for (uint256 i = 0; i < vesters.length; i++) {
        require(
            amounts[i] != 0,
            "LinearVesting::constructor: Incorrect Amount Specified"
        );
        vest[vesters[i]].amount = amounts[i];
        total = total + amounts[i];
    }
    require(
        total == _TEAM_ALLOCATION,
        "LinearVesting::constructor: Invalid Vest Amounts Specified"
    );

    transferOwnership(address(_vader));
}