Omniscia Keyko Audit

CommunityAdminImplementation Manual Review Findings

CommunityAdminImplementation Manual Review Findings

CAI-01M: Overly Centralized Asset Control

Description:

The CommunityAdminImplementation has complete control over all ERC20 funds spread across the communities and the contract itself.

Example:

contracts/community/CommunityAdminImplementation.sol
379/**
380 * @notice Transfers an amount of an ERC20 from this contract to an address
381 *
382 * @param token_ address of the ERC20 token
383 * @param to_ address of the receiver
384 * @param amount_ amount of the transaction
385 */
386function transfer(
387    IERC20 token_,
388    address to_,
389    uint256 amount_
390) external override onlyOwner nonReentrant {
391    token_.safeTransfer(to_, amount_);
392

393    emit TransferERC20(address(token_), to_, amount_);
394}
395

396/**
397 * @notice Transfers an amount of an ERC20 from  community to an address
398 *
399 * @param community_ address of the community
400 * @param token_ address of the ERC20 token
401 * @param to_ address of the receiver
402 * @param amount_ amount of the transaction
403 */
404function transferFromCommunity(
405    ICommunity community_,
406    IERC20 token_,
407    address to_,
408    uint256 amount_
409) external override onlyOwner nonReentrant {
410    community_.transfer(token_, to_, amount_);
411}

Recommendation:

We advise this trait to be carefully evaluated as it is a significant single-point-of-failure (SPoF) of the system.

Alleviation:

The Keyko team has stated that ownership of the contract is meant to be transferred to a Timelock address representing governance and as such, the code performs as expected thereby nullifying this exhibit.

CAI-02M: Potential Duplication of Community Migration

Description:

The migrateCommunity function does not validate whether the community has already been migrated and as such can be invoked multiple times for the same community. Additionally, it does not validate that the previousCommunity_ was indeed a valid community contract.

Example:

contracts/community/CommunityAdminImplementation.sol
266/**
267 * @notice Migrates a community by deploying a new contract.
268 *
269 * @param firstManager_ address of the community first manager. Will be able to add others
270 * @param previousCommunity_ address of the community to be migrated
271 * @param managerBlockList_    Addresses of managers that have to not be managers
272 */
273function migrateCommunity(
274    address firstManager_,
275    ICommunity previousCommunity_,
276    address[] memory managerBlockList_
277) external override onlyOwner nonReentrant {
278    _communities[address(previousCommunity_)] = CommunityState.Removed;
279    require(
280        address(previousCommunity_) != address(0),
281        "CommunityAdmin::migrateCommunity: NOT_VALID"
282    );
283

284    uint256 maxClaim = isCommunityNewType(previousCommunity_)
285        ? previousCommunity_.getInitialMaxClaim()
286        : previousCommunity_.maxClaim();
287

288    ICommunity community = ICommunity(
289        deployCommunity(
290            firstManager_,
291            previousCommunity_.claimAmount(),
292            maxClaim,
293            previousCommunity_.decreaseStep(),
294            previousCommunity_.baseInterval(),
295            previousCommunity_.incrementInterval(),
296            previousCommunity_.minTranche(),
297            previousCommunity_.maxTranche(),
298            previousCommunity_,
299            managerBlockList_
300        )
301    );
302    require(address(community) != address(0), "CommunityAdmin::migrateCommunity: NOT_VALID");
303

304    if (isCommunityNewType(previousCommunity_)) {
305        uint256 balance = _cUSD.balanceOf(address(previousCommunity_));
306        previousCommunity_.transfer(_cUSD, address(community), balance);
307    }
308

309    _communities[address(community)] = CommunityState.Valid;
310    _communityList.add(address(community));
311

312    emit CommunityMigrated(firstManager_, address(community), address(previousCommunity_));
313}

Recommendation:

We advise a require check to be imposed to ensure that the previousCommunity_ state was valid in the _communities mapping before adjusting it to Removed.

Alleviation:

A migration status is now properly set for the previous community ensuring that it cannot be re-migrated and addressing this exhibit.

CAI-03M: Inexistent Initialization of Re-Entrancy Guard

Description:

The re-entrancy guard contract is never initialized in the initialize call chain, thus causing it to be improperly set up.

Example:

contracts/community/CommunityAdminImplementation.sol
124/**
125 * @notice Used to initialize a new CommunityAdmin contract
126 *
127 * @param communityTemplate_    Address of the Community implementation
128 *                              used for deploying new communities
129 * @param cUSD_                 Address of the cUSD token
130 */
131function initialize(ICommunity communityTemplate_, IERC20 cUSD_) external override initializer {
132    __Ownable_init();
133

134    _communityTemplate = communityTemplate_;
135    _cUSD = cUSD_;
136

137    _communityProxyAdmin = new ProxyAdmin();
138}

Recommendation:

We advise it to be properly initialized by invoking its __ReentrancyGuard_init function within the initialize function of the CommunityAdminImplementation contract.

Alleviation:

The re-entrancy guard is now properly initialised in the initialize call chain.

CAI-04M: Inexistent Validation of Community Removal

Description:

The removal of a community should only be performed once, however, the removeCommunity function performs no such validation.

Example:

contracts/community/CommunityAdminImplementation.sol
355/**
356 * @notice Removes an existing community. All community funds are transferred to the treasury
357 *
358 * @param community_ address of the community
359 */
360function removeCommunity(ICommunity community_) external override onlyOwner nonReentrant {
361    _communities[address(community_)] = CommunityState.Removed;
362    emit CommunityRemoved(address(community_));
363    community_.transfer(_cUSD, address(_treasury), _cUSD.balanceOf(address(community_)));
364}

Recommendation:

We advise the removeCommunity function to properly validate the previous state of the community_ being removed.

Alleviation:

State validation for the removal of a community is now performed properly.

CAI-05M: Potentially Incorrect Tranche Calculation

Description:

The tranche calculation mechanism does not factor into account the existing _cUSD balance of the community which should not be transferred again to it.

Example:

contracts/community/CommunityAdminImplementation.sol
366/**
367 * @dev Funds an existing community if it hasn't enough funds
368 */
369function fundCommunity() external override onlyCommunities {
370    require(
371        _cUSD.balanceOf(msg.sender) <= ICommunity(msg.sender).minTranche(),
372        "CommunityAdmin::fundCommunity: this community has enough funds"
373    );
374    uint256 trancheAmount = calculateCommunityTrancheAmount(ICommunity(msg.sender));
375

376    transferToCommunity(ICommunity(msg.sender), trancheAmount);
377}

Recommendation:

We advise the transferToCommunity call to have the _cUSD.balanceOf(msg.sender) evaluation subtracted from the trancheAmount being transferred.

Alleviation:

The codebase has been updated to properly subtract the balance of the community from the transfer in a graceful manner, resulting in a no-op in case the calculated tranche amount exceeds the balance of the community.

Prev

Community.sol (COM-M)

Next

DonationMinerImplementation.sol (DMI-M)

On this page