Kapital-DAO - PlayGround Labs

In RewardsLocker.sol, Staking.sol, Vesting.sol contracts, return values from external transfer calls are not checked. It should be noted that the token is not reverted on failure and returns false. If one of these tokens is used, a deposit would not be reverted if the transfer failed and an attacker could deposit tokens for free.

Code Location

RewardsLocker.sol

    function collectRewards(uint256 lockAgreementId) external {
        LockAgreement storage lockAgreement = lockAgreements[msg.sender][
            lockAgreementId
        ];
        // make sure the beneficiary waits before collecting the rewards
        require(
            block.timestamp >= lockAgreement.availableTimestamp,
            "Collection too early"
        );
        // make sure the beneficiary has not already collected the rewards
        require(!lockAgreement.collected, "Already collected");
        // set `collected` to true, so the beneficiary cannot withdraw again
        lockAgreement.collected = true;
        // update voting weight
        weightKAP[msg.sender] -= lockAgreement.amount;
        // transfer `amount` KAP to the beneficiary
        kapToken.transfer(msg.sender, lockAgreement.amount);
    }

RewardsLocker.sol

    function transferKap(address to, uint256 amount) external {
        bool senderIsGovernance = (msg.sender ==
            governanceRegistry.governance());
        bool authorized = senderIsGovernance || hasRole(KAP_SAVER, msg.sender);
        require(authorized, "Access denied");
        kapToken.transfer(to, amount);
    }

Staking.sol

    function _transferFromAndReturnAddAmount(
        address staker,
        uint256 inputAmount
    ) internal returns (uint256) {
        uint256 previousBalance = asset.balanceOf(address(this));
        asset.transferFrom(staker, address(this), inputAmount);
        return asset.balanceOf(address(this)) - previousBalance;
    }

Staking.sol

    function unstake(uint256 removeAmount, uint256 stakingAgreementId)
        external
    {
        // update {multipliedTotalRewardsPerWeightLastSync} if necessary
        if (block.timestamp > lastSync) {
            sync();
        }

        Staker storage staker = stakers[msg.sender];
        StakingAgreement storage stakingAgreement = staker.stakingAgreements[
            stakingAgreementId
        ];

        require(
            (removeAmount > 0) && (removeAmount <= stakingAgreement.amount),
            "Staking: Invalid amount"
        );
        require(
            block.timestamp >= stakingAgreement.lockEnd,
            "Staking: Too early"
        );

        asset.transfer(msg.sender, removeAmount);
        staker.totalAmount -= Math.toUint112(removeAmount);
        stakingAgreement.amount -= Math.toUint112(removeAmount);

        uint256 unstakeWeight = _calculateStakeWeight(
            stakingAgreement.lockEnd - stakingAgreement.lockStart,
            removeAmount
        );
        totalStakingWeight -= unstakeWeight;
        staker.totalWeight -= Math.toUint136(unstakeWeight);
        // looking at {claimRewards}, the above line has instantaneously
        // decreased `claimedRewards` by
        // `(unstakeWeight * multipliedTotalRewardsPerWeightLastSync) / REWARDS_PER_WEIGHT_MULTIPLIER`.
        // we therefore need to give this amount back to the user in the form
        // of adding to `staker.addRewards`.
        staker.addRewards +=
            (unstakeWeight * multipliedTotalRewardsPerWeightLastSync) /
            REWARDS_PER_WEIGHT_MULTIPLIER;

        emit Unstake(msg.sender, removeAmount);
    }

Vesting.sol

    function collect(uint256 vestingAgreementId) external {
        VestingAgreement storage vestingAgreement = vestingAgreements[
            msg.sender
        ][vestingAgreementId];
        // make sure the vesting period has started
        require(
            block.timestamp > vestingAgreement.vestStart,
            "Vesting not started"
        );
        // calculate portion of `totalAmount` currently unlocked
        uint256 amountUnlocked;
        // if {VESTING_PERIOD} has passed, the entire `totalAmount` is unlocked
        if (block.timestamp >= (vestingAgreement.vestStart + VESTING_PERIOD)) {
            amountUnlocked = vestingAgreement.totalAmount;
        }
        // otherwise, we find the portion of `totalAmount` currently available
        else {
            amountUnlocked =
                (vestingAgreement.totalAmount *
                    (block.timestamp - vestingAgreement.vestStart)) /
                VESTING_PERIOD;
        }
        // make sure some of `amountUnlocked` has not yet been collected
        require(
            amountUnlocked > vestingAgreement.amountCollected,
            "Collection limit reached"
        );
        // calculate amount available for collection
        uint256 collectionAmount = amountUnlocked -
            vestingAgreement.amountCollected;
        // update balance
        balances[msg.sender] -= collectionAmount;
        // update voting weight
        weightKAP[delegates[msg.sender]] -= collectionAmount;
        // update vesting agreement to indicate a collection has been performed
        vestingAgreement.amountCollected += SafeCast.toUint96(collectionAmount);
        // transfer KAP tokens available for collection
        kapToken.transfer(msg.sender, collectionAmount);
    }

Vesting.sol

    function createVestingAgreement(
        address beneficiary,
        uint256 vestStart,
        uint256 amount
    ) external onlyRole(VESTING_CREATOR) {
        // caller provides KAP for the vesting agreement
        kapToken.transferFrom(msg.sender, address(this), amount);
        // update balance
        balances[beneficiary] += amount;
        // update delegate voting weight
        weightKAP[delegates[beneficiary]] += amount;
        // push a new vesting agreement for the beneficiary
        vestingAgreements[beneficiary].push(
            VestingAgreement({
                vestStart: SafeCast.toUint64(vestStart),
                totalAmount: SafeCast.toUint96(amount),
                amountCollected: SafeCast.toUint96(0)
            })
        );
    }

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the team now uses OpenZeppelin’s SafeERC20 to perform the transfers. However, the team claims that the tokens used in the Kapital DAO (KAP token and Uniswap V2 Pair) revert on failure, and otherwise return a hardcoded value of true in the above code.

One of the contracts included in the scope of Playground Labs Kapital-DAO was identified as missing a nonReentrant guard. In this function, persistent state read/write after an external call is identified, making it vulnerable to a Reentrancy attack.

• The Staking.sol contract function unstake is missing nonReentrant guard.

To protect against cross-function reentrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called “nonReentrant” that guards the function with a mutex against the Reentrancy attacks.

Code Location

Staking.sol

    function unstake(uint256 removeAmount, uint256 stakingAgreementId)
        external
    {
        // update {multipliedTotalRewardsPerWeightLastSync} if necessary
        if (block.timestamp > lastSync) {
            sync();
        }

        Staker storage staker = stakers[msg.sender];
        StakingAgreement storage stakingAgreement = staker.stakingAgreements[
            stakingAgreementId
        ];

        require(
            (removeAmount > 0) && (removeAmount <= stakingAgreement.amount),
            "Staking: Invalid amount"
        );
        require(
            block.timestamp >= stakingAgreement.lockEnd,
            "Staking: Too early"
        );

        asset.transfer(msg.sender, removeAmount);
        staker.totalAmount -= Math.toUint112(removeAmount);
        stakingAgreement.amount -= Math.toUint112(removeAmount);

        uint256 unstakeWeight = _calculateStakeWeight(
            stakingAgreement.lockEnd - stakingAgreement.lockStart,
            removeAmount
        );
        totalStakingWeight -= unstakeWeight;
        staker.totalWeight -= Math.toUint136(unstakeWeight);
        // looking at {claimRewards}, the above line has instantaneously
        // decreased `claimedRewards` by
        // `(unstakeWeight * multipliedTotalRewardsPerWeightLastSync) / REWARDS_PER_WEIGHT_MULTIPLIER`.
        // we therefore need to give this amount back to the user in the form
        // of adding to `staker.addRewards`.
        staker.addRewards +=
            (unstakeWeight * multipliedTotalRewardsPerWeightLastSync) /
            REWARDS_PER_WEIGHT_MULTIPLIER;

        emit Unstake(msg.sender, removeAmount);
    }

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the code now follows the checks-effects-interactions pattern. However, the team claims that the tokens used in Kapital DAO (KAP token and Uniswap V2 Pair) are not vulnerable to reentrancy into above code.

In the Governance.sol contract, the proposeCooldown state variable is not initialized, it defaults to 0 value, and the variable is considered in the other calculation progresses, i.e., require( timestamp >= latestPropose[msg.sender] + proposeCooldown, "Governance: Propose Cooldown"); in the propose function. If a variable must be initialized to zero, explicitly set it to zero to improve code readability.

Code Location

Governance.sol

 uint24 public proposeCooldown;

Governance.sol

    function propose(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory data,
        bool[] memory isDelegateCall,
        WeightSources memory weightSources
    ) external returns (uint256) {
        // Ensure msg.sender has enough voting weight
        (uint256 weightKAP, uint256 weightLP) = getWeights(weightSources);
        require(
            weightKAP + convertLP(weightLP) >= threshold,
            "Governance: Insufficient weight"
        );
        // Make sure haven't already created a proposal within cooldown period, Propose CoolDown
        uint256 timestamp = block.timestamp;
        require(
            timestamp >= latestPropose[msg.sender] + proposeCooldown,
            "Governance: Propose Cooldown"
        );

        // Add new proposal

SOLVED: The Playground labs team solved the above issue in the commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the team initializes proposeCooldown to 3 days (same as the voting period) so that the voting process is never overwhelmed by repeated proposals.

Constructor Initialize propseCooldown

        // implicitly set propseCooldown to be the voting period
        proposeCooldown = _waitTo.endVote - _waitTo.startVote;

External calls within a loop increase Gas usage or can lead to a denial of service attack. In the Governance.sol contract functions discovered there is a for loop on the i variable that iterates through the weightSources.kapSources.length and weightSources.lpSources.length array length, and this loop has external calls within a loop. If this integer evaluates to extremely large numbers, this can cause a DoS.

Code Location

Governance.sol

    function getWeights(WeightSources memory weightSources)
        public
        view
        returns (uint256 weightKAP, uint256 weightLP)
    {
        // Calc KAP voting weight
        for (uint256 i = 0; i < weightSources.kapSources.length; i++) {
            if (weightSources.kapSources[i]) {
                weightKAP += IKAPSource(weightSourcesKAP[i]).weightKAP(
                    msg.sender
                );
            }
        }
        // Calc LP voting weight
        for (uint256 i = 0; i < weightSources.lpSources.length; i++) {
            if (weightSources.lpSources[i]) {
                weightLP += ILPSource(weightSourcesLP[i]).weightLP(msg.sender);
            }
        }
    }

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. The team modifies the WeightSources and getWeights structs; as a result, the user can now choose specific array indices in weightSourcesKAP and weightSourcesLP. Furthermore, the team added that in the unlikely case that weightSourcesKAP and weightSourcesLP are very long, the user can choose a relatively small number of WeightSources to loop through.

The return value of an external call is not stored in a local or state variable. In the Transactor.sol contract, there is an instance where an external method is called, and the return value is ignored.

Code Location

Transactor.sol

    function _transact(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory data,
        bool[] memory isDelegateCall
    ) internal {
        require(targets.length > 0, "Invalid array length");
        require(targets.length == values.length, "Array length mismatch");
        require(targets.length == data.length, "Array length mismatch");
        require(
            targets.length == isDelegateCall.length,
            "Array length mismatch"
        );

        for (uint256 i = 0; i < targets.length; i++) {
            if (isDelegateCall[i]) {
                Address.functionDelegateCall(targets[i], data[i]);
            } else {
                Address.functionCallWithValue(targets[i], data[i], values[i]);
            }
        }
    }

RISK ACCEPTED: The Playground labs team accept the risk of this finding. Furthermore, the team claims that the team does not have any function-specific return value due to not having information about which functions can be called in advance. However, the team added that the Address contract confirms that success == true and reverts otherwise.

The supplied newGovernance is not being validated before the transfer of ownership, even though the governance access control is in place. If configured incorrectly, it will lock all governance functionality.

GovernanceRegistry.sol

    function changeGovernance(address newGovernance) external {
        require(msg.sender == governance, "Only governance");
        governance = newGovernance;
    }

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the team added a two-step governance change process. Furthermore, the team also added additional validation of the new governance address.

Updated changeGovernance

    function changeGovernance(address newGovernance) external {
        require(msg.sender == governance, "Only governance");
        require(
            newGovernance != address(0) && newGovernance != governance,
            "Invalid governance"
        );

        IGovernance _newGovernance = IGovernance(newGovernance);
        require(_newGovernance.votingPeriod() > 0);

        appointedNewGovernance = newGovernance;
    }

In the Governance.sol contract, it is noted that the vote function lacks the legitimacy of msg.sender if it is a valid voter. As a result, an unknown EOA or contract may cast a vote; therefore, the result of the vote can be manipulated, although it is unlikely since there are no benefits for said voter.

Code Location

Governance.sol

    function vote(
        uint256 proposalID,
        bool yay,
        WeightSources memory weightSources
    ) external {
        Proposal storage proposal = proposals[proposalID];

        // Enforce voting window, Voting Window
        require(_checkVoteWindow(proposal), "Governance: Voting window");

        // Mark msg.sender as having voted, Already Voted
        require(!proposal.hasVoted[msg.sender], "Governance: Already voted");
        proposal.hasVoted[msg.sender] = true;

        (uint256 weightKAP, uint256 weightLP) = getWeights(weightSources);

        // Add to vote counts
        require(weightLP <= type(uint112).max, "Governance: uint112(weightLP)");
        if (yay) {
            proposal.yaysKAP += SafeCast.toUint96(weightKAP);
            proposal.yaysLP += uint112(weightLP);
        } else {
            proposal.naysKAP += SafeCast.toUint96(weightKAP);
            proposal.naysLP += uint112(weightLP);
        }

        // Record that `msg.sender` last voted at this timestamp
        lastVoted[msg.sender] = block.timestamp;

        emit Voted(msg.sender, proposalID, yay, weightKAP, weightLP);
    }

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a solution, the team a descriptive requirement require(weightKAP > 0 || weightLP > 0, "Governance: Zero weight") to prevent zero-weight accounts from voting unnecessarily. Furthermore, the team claims that the team allows contracts, particularly multisig wallets, to stake and vote. And the only requirement to propose is to meet the threshold that the team has currently initialized at 0.65% of the total KAP supply, and there is no whitelist of proposals.

During a manual review, the use of block.timestamp in some Playground Labs Kapital-DAO contracts were observed. Contract developers should note that this does not mean the current time. Miners can influence the value of block.timestamp to some degree, so testers should be warned that this may come at some risk if miners collude in time manipulation to influence price oracles. It is important to follow the 15-second rule, i.e., if the contract is not based on an interval of less than 15-seconds, it is fine to use block.timestamp.

Code Location

Vesting.sol

#101: block.timestamp > vestingAgreement.vestStart,
#107: if (block.timestamp >= (vestingAgreement.vestStart + VESTING_PERIOD)) {
#114: (block.timestamp - vestingAgreement.vestStart)) /
#148: block.timestamp > oldDelegateLastVoted + votingPeriod,

Governance.sol

#152: uint256 timestamp = block.timestamp;
#187: uint256 timeElapsed = block.timestamp - proposal.proposeTime;
#224: lastVoted[msg.sender] = block.timestamp;
#239: uint256 timeElapsed = block.timestamp - proposal.proposeTime;
#323: emit ProposalExecuted(msg.sender, proposalID, block.timestamp);
#350: (block.timestamp - proposal.proposeTime);

Staking.sol

#148: (block.timestamp <= (lastStaked[voter] + votingPeriod))
#161: if (block.timestamp <= rewardsStart) {
#182: if (block.timestamp > lastSync) {
#185: lastStaked[msg.sender] = block.timestamp;
#230: if (block.timestamp > lastSync) {
#244: block.timestamp >= stakingAgreement.lockEnd,
#288: if (block.timestamp > lastSync) {
#295: if (block.timestamp <= rewardsStart) {
#320: require(block.timestamp > lastSync, "Staking: Already syncd");
#323: if (block.timestamp > rewardsRules[rewardsRuleIndex].timeEnd) {
#419: if (block.timestamp <= rewardsRule.timeEnd) {
#470: uint256 lockStart = block.timestamp <= rewardsStart

RewardsLocker.sol

#97: block.timestamp >= lockAgreement.availableTimestamp,

RISK ACCEPTED: The Playground labs team accepted the risk of this finding.

Several instances found where the address validation is missing. A zero address validation failure was found when assigning user-supplied address values to state variables directly.

• In contract GovernanceRegistry.sol:

  • changeGovernance lacks a zero address check on newGovernance.
  • constructor lacks a zero address check on initialGovernance.

• In contract MultisigFund.sol:

  • constructor lacks a zero address check on _multisig.

• In contract RewardsLocker.sol:

  • constructor lacks a zero address check on _kapStakingPool, _kapEthStakingPool.

• In contract Vesting.sol:

  • constructor lacks a zero address check on _teamMultisig.

• changeGovernance lacks a zero address check on newGovernance.

• constructor lacks a zero address check on initialGovernance.

• constructor lacks a zero address check on _multisig.

• constructor lacks a zero address check on _kapStakingPool, _kapEthStakingPool.

• constructor lacks a zero address check on _teamMultisig.

Code Location

Zero Address Validation is missing before assigning addresses to these state variables.

GovernanceRegistry.sol

 governance = initialGovernance (#20)
 governance = newGovernance (#30)

MultisigFund.sol

 multisig = _multisig (#18)

RewardsLocker.sol

 kapStakingPool = _kapStakingPool (#34)
 kapEthStakingPool = _kapEthStakingPool (#35)

Vesting.sol

 teamMultisig = _teamMultisig (#32)

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the team added zero address checks for the above code.

Solidity's integer division could be truncated. As a result, precision loss can sometimes be avoided by multiplying before dividing, although the manual implementation of the precision/decimal calculation is taken care of by the developer. In the set of smart contracts, there is an instance where the division is done before the multiplication.

Code Location

Governance.sol

        uint256 priceETH = (_cumulative() - proposal.priceCumulativeLast) /
            (block.timestamp - proposal.proposeTime);
        uint256 reserveKAP = Math.sqrt(k * priceETH);

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the contract now performs multiplication before division.

In public functions, array arguments are immediately copied into memory, while external functions can read directly from calldata. Reading calldata is cheaper than allocating memory. Public functions need to write arguments to memory because public functions can be called internally. Internal calls are passed internally via pointers to memory. Thus, the function expects its arguments to be located in memory when the compiler generates the code for an internal function. Also, methods do not necessarily have to be public if they are only called within the contract; in such case, they should be marked as internal.

Code Location

Below are the smart contracts and their corresponding functions affected: \textbf{\underline{Staking.sol}:} getStakingAgreementsLength()

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the team has changed getStakingAgreementsLength() to external.

Exponentiation is more expensive than the following implementation.

1e18 is more cheap than 10**18.

Code Location

Token.sol

    constructor() ERC20("Kapital DAO Token", "KAP") {
        uint256 uiKapTotalSupply = 10**9;
        _mint(msg.sender, uiKapTotalSupply * (10**decimals()));
    }

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the team replaced 10**9 with 1e9.

In the loop below, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++.

Code Location

Governance.sol

    function getWeights(WeightSources memory weightSources)
        public
        view
        returns (uint256 weightKAP, uint256 weightLP)
    {
        // Calc KAP voting weight
        for (uint256 i = 0; i < weightSources.kapSources.length; i++) {
            if (weightSources.kapSources[i]) {
                weightKAP += IKAPSource(weightSourcesKAP[i]).weightKAP(
                    msg.sender
                );
            }
        }
        // Calc LP voting weight
        for (uint256 i = 0; i < weightSources.lpSources.length; i++) {
            if (weightSources.lpSources[i]) {
                weightLP += ILPSource(weightSourcesLP[i]).weightLP(msg.sender);
            }
        }
    }

Transactor.sol

    function _transact(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory data,
        bool[] memory isDelegateCall
    ) internal {
        require(targets.length > 0, "Invalid array length");
        require(targets.length == values.length, "Array length mismatch");
        require(targets.length == data.length, "Array length mismatch");
        require(
            targets.length == isDelegateCall.length,
            "Array length mismatch"
        );

        for (uint256 i = 0; i < targets.length; i++) {
            if (isDelegateCall[i]) {
                Address.functionDelegateCall(targets[i], data[i]);
            } else {
                Address.functionCallWithValue(targets[i], data[i], values[i]);
            }
        }
    }

For example, based on the following test contract:

Test.sol

//SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i++) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

We can see the difference in the gas costs:

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, for loops now uses ++i.

Reading the length of the array at each iteration of the loop requires 6 gas (3 for mload and 3 to place memory_offset) onto the stack. Caching the length of the array on the stack saves about 3 gas per iteration.

Code Location

Governance.sol

    function getWeights(WeightSources memory weightSources)
        public
        view
        returns (uint256 weightKAP, uint256 weightLP)
    {
        // Calc KAP voting weight
        for (uint256 i = 0; i < weightSources.kapSources.length; i++) {
            if (weightSources.kapSources[i]) {
                weightKAP += IKAPSource(weightSourcesKAP[i]).weightKAP(
                    msg.sender
                );
            }
        }
        // Calc LP voting weight
        for (uint256 i = 0; i < weightSources.lpSources.length; i++) {
            if (weightSources.lpSources[i]) {
                weightLP += ILPSource(weightSourcesLP[i]).weightLP(msg.sender);
            }
        }
    }

Transactor.sol

    function _transact(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory data,
        bool[] memory isDelegateCall
    ) internal {
        require(targets.length > 0, "Invalid array length");
        require(targets.length == values.length, "Array length mismatch");
        require(targets.length == data.length, "Array length mismatch");
        require(
            targets.length == isDelegateCall.length,
            "Array length mismatch"
        );

        for (uint256 i = 0; i < targets.length; i++) {
            if (isDelegateCall[i]) {
                Address.functionDelegateCall(targets[i], data[i]);
            } else {
                Address.functionCallWithValue(targets[i], data[i], values[i]);
            }
        }
    }

SOLVED: The Playground labs team solved the above issue in commit 35fb92524b83ff8197a7127f7c9819317ac7ea92. As a result, the contract cache the array length.