BaseVesting Manual Review Findings

BVG-01M: ECDSA Signature Malleability

Description:

The signature recovery mechanism employed by isValidData does not account for malleable signatures by validating its input v and s arguments.

Example:

Copy
78function isValidData(
79    address addr,
80    uint256 portionLP,
81    uint256 portionNative,
82    uint deadline,
83    uint8 v,
84    bytes32 r,
85    bytes32 s
86) public returns (bool) {
87    bytes32 message = keccak256(
88        abi.encodePacked(
89            address(this),
90            addr,
91            portionLP,
92            portionNative,
93            nonces[addr].current(),
94            deadline
95        )
96    );
97
98    address sender = ecrecover(message, v, r, s);
99    if (trustedSigner[sender]) {
100        nonces[addr].increment();
101        return true;
102    } else {
103        return false;
104    }
105}

Recommendation:

We advise such sanitization to be introduced by ensuring that v is equal to 27 or 28 and that s is a point in the lower half of the secp256k1 integer order (i.e. exactly how OpenZeppelin's ECDSA library conducts this check) to prevent malleable signatures from being replayed as the secp256k1 curve is mirrored on the X axis. Although this trait is guaranteed by the usage of a nonce system, it is still best practice to prevent these signatures. Additionally, we should note that incorrect signatures will yield the zero address (0x0) allowing the code block to continue, however, the zero address can never become a trustedSigner due to the check imposed in changePermission and thus a check here is not necessary.

Alleviation:

The two require checks that we requested be included in the codebase were done so thus preventing incorrectly crafted signatures from being replayed.

BVG-02M: Improper Edge Case

Description:

The getRewardBalance function validates whether the amount to be rewarded exceeds the currently held balance and yields 0 in case that condition is evaluated as true.

Example:

Copy
149uint256 reward = _getRewardBalance(percenageLP, percentageNative);
150Investor storage investor = investorInfo[beneficiary];
151uint256 balance = token.balanceOf(address(this));
152if (reward <= investor.paidAmount) {
153    return 0;
154} else {
155    uint256 amountToPay = reward.sub(investor.paidAmount);
156    if (amountToPay >= balance) {
157        return 0;
158    }
159    return amountToPay;
160}

Recommendation:

We advise that balance is returned instead to accommodate for mathematical truncations, similarly to how most DeFi contracts handle payouts such as SushiSwap.

Alleviation:

The getRewardBalance function now properly yields the available balance of the contract should amountToPay exceed it.

BVG-03M: Inconsistent Edge Case Handling

Description:

The _withdrawReward will fail to execute if the amount to be rewarded is greater than the current balance of the contract even by a single unit.

Example:

Copy
170uint256 balance = token.balanceOf(address(this));
171require(reward > investor.paidAmount, "No rewards available");
172uint256 amountToPay = reward.sub(investor.paidAmount);
173require(amountToPay <= balance, "The rewards are over");

Recommendation:

We advise a proper edge-case to be handled here whereby the amountToPay exceeding balance should simply pay out the balance to account for truncations.

Alleviation:

The require check was replaced by an if block that assigns the currently held balance as the amountToPay to properly account for the aforementioned edge case thus alleviating this exhibit.

BVG-04M: Inexistent Self-Sufficiency

Description:

The constructor of the BaseVesting contract does not set any configurational values of the contract.

Example:

Copy
21IERC20 public token;
22uint256 public everyDayReleasePercentage;
23uint256 public periods;
24uint256 public startDate;
25uint256 public totalAllocatedAmount;
26uint256 public tokensForLP;
27uint256 public tokensForNative;
28uint256 public vestingDuration;
29uint256 public vestingTimeEnd;
30
31event RewardPaid(address indexed investor, uint256 amount);
32
33mapping(address => Counters.Counter) public nonces;
34mapping(address => bool) public trustedSigner;
35mapping(address => Investor) public investorInfo;
36
37constructor(address signer_) {
38    require(signer_ != address(0), "Invalid signer address");
39    trustedSigner[signer_] = true;
40}

Recommendation:

We advise the assignments to be relocated from inherited contracts to the BaseVesting contract constructor to firstly ensure that input sanitization is done properly and secondly to allow the usage of mutability optimizations such as the immutable specifier for variables that are only set once.

Alleviation:

As with the AdvanceVesting implementation, the configurational values of the BaseVesting contract are now properly sanitized and assigned during the contract's constructor. Additionally, the immutable specifier was introduced where applicable greatly optimizing the gas cost of the contract.

BVG-05M: Overly Centralized Control

Description:

The emergencyTokenWithdraw method permits the owner to withdraw any token held by the contract as a safety measure.

Example:

Copy
54/**
55 * @dev emergency tokens withdraw
56 * @param tokenAddress_ token address
57 * @param amount amount to withdraw
58 */
59function emergencyTokenWithdraw(address tokenAddress_, uint256 amount)
60    external
61    onlyOwner
62{
63    IERC20 tokenAddress = IERC20(tokenAddress_);
64    require(
65        tokenAddress.balanceOf(address(this)) >= amount,
66        "Insufficient tokens balance"
67    );
68    tokenAddress.transfer(msg.sender, amount);
69}

Recommendation:

We advise this particular function to be omitted as it defeats the purpose of the vesting period and can override the vesting period at will. Alternatively, we advise it to solely be accessible beyond the vesting period's conclusion.

Alleviation:

The latter of our recommendations was heeded and the function was made accessible after the vestingTimeEnd has passed thus ensuring that it can be executed expectedly.