Omniscia Keyko Audit

Community Manual Review Findings

Community Manual Review Findings

COM-01M: Improper Role Management System

TypeSeverityLocation
Standard ConformityMediumCommunity.sol:L584, L598

Description:

The AccessControl contract is being mis-utilized by the Community implementation as it invokes the _setupRole to set a role by a manager or community administrator, however, the removal of a manager is performed by the revokeRole function which itself has its own access control imposed and will prevent it from ever being successfully invoked as it can only be invoked by the role administrator which is not set up anywhere in the system.

Example:

contracts/community/Community.sol
573/**
574 * @notice Adds a new manager
575 *
576 * @param account_ address of the manager to be added
577 */
578function addManager(address account_)
579 external
580 override
581 onlyManagersOrCommunityAdmin
582 eligibleManager(account_)
583{
584 _setupRole(MANAGER_ROLE, account_);
585 emit ManagerAdded(msg.sender, account_);
586}
587
588/**
589 * @notice Remove an existing manager
590 *
591 * @param account_ address of the manager to be removed
592 */
593function removeManager(address account_) external override onlyManagersOrCommunityAdmin {
594 require(
595 account_ != address(_communityAdmin),
596 "Community::removeManager: You are not allow to remove communityAdmin"
597 );
598 revokeRole(MANAGER_ROLE, account_);
599 emit ManagerRemoved(msg.sender, account_);
600}

Recommendation:

We advise the role management system of the contract to be revised as it will be inoperable in its current state.

Alleviation:

The revocation system has been revised to impose a role sanitization on the account being removed, however, the revokeRole function appears to be invoked directly once again. We advise the Keyko team to clarify as to whether this is intended behaviour.

COM-02M: Inexistent Prevention of cUSD Transfer

TypeSeverityLocation
Logical FaultMediumCommunity.sol:L812-L827

Description:

The transfer function allows the owner to transfer an arbitrary token out of the system.

Example:

contracts/community/Community.sol
812/**
813 * @notice Transfers an amount of an ERC20 from this contract to an address
814 *
815 * @param token_ address of the ERC20 token
816 * @param to_ address of the receiver
817 * @param amount_ amount of the transaction
818 */
819function transfer(
820 IERC20 token_,
821 address to_,
822 uint256 amount_
823) external override onlyOwner nonReentrant {
824 token_.safeTransfer(to_, amount_);
825
826 emit TransferERC20(address(token_), to_, amount_);
827}

Recommendation:

We strongly advise a sanitization to be set in place to prevent cUSD from being transferrable as that is an integral system component and can cause the various variables of the system to be desynchronized. As the functionality is needed by the

Alleviation:

The Keyko team has stated that this is intended behaviour of the contract as the owner is meant to be represented by a Timelock contract that in turn will only execute governance votes. As a result, we consider this finding null.

COM-03M: Inexistent Validation of Max Claim & Step Arguments

Description:

Based on the implementation of _changeBeneficiaryState, the _maxClaim variable should be a non-zero number that is wholly divisible by the _decreaseStep variable, a validation that is not imposed by the functions that adjust those variables.

Example:

contracts/community/Community.sol
892/**
893 * @notice Changes the state of a beneficiary
894 *
895 * @param beneficiary address of the beneficiary
896 * @param newState_ new state
897 */
898function _changeBeneficiaryState(Beneficiary storage beneficiary, BeneficiaryState newState_)
899 internal
900{
901 if (beneficiary.state == newState_) {
902 return;
903 }
904
905 _beneficiaryList.add(msg.sender);
906
907 if (newState_ == BeneficiaryState.Valid) {
908 _validBeneficiaryCount++;
909 _maxClaim -= _decreaseStep;
910 } else if (beneficiary.state == BeneficiaryState.Valid) {
911 _validBeneficiaryCount--;
912 _maxClaim += _decreaseStep;
913 }
914
915 beneficiary.state = newState_;
916}

Recommendation:

We advise such sanitization to be imposed as incorrectly set variables will cause the contract to be inoperable in relation to the beneficiary states. To achieve this, the modulo (%) operator should be used between _maxClaim and _decreaseStep as well as a non-zero validation for the _maxClaim itself.

Alleviation:

The recommended validation was introduced by performing the subtraction and validating that the result is above or equal to (>=) the claimAmount instead, thereby alleviating this exhibit.

COM-04M: Potentially Prohibitive Migration Mechanism

Description:

The claims calculation based on the legacy community implementation requires that the previousLastInterval is greater-than-or-equal-to _baseInterval, a trait that is not guaranteed and can cause the migration to fail due to an underflow.

Example:

contracts/community/Community.sol
856uint256 previousLastInterval = oldCommunity.lastInterval(msg.sender);
857_changeBeneficiaryState(
858 beneficiary,
859 BeneficiaryState(oldCommunity.beneficiaries(msg.sender))
860);
861beneficiary.lastClaim = oldCommunity.cooldown(msg.sender) - previousLastInterval;
862beneficiary.claimedAmount = oldCommunity.claimed(msg.sender);
863beneficiary.claims = (previousLastInterval - _baseInterval) / _incrementInterval + 1;

Recommendation:

We advise an alternative or safe calculation mechanism to be devised here for the number of claims a legacy user has made as the current one is prone to error.

Alleviation:

Graceful handling was introduced to the codebase by introducing a new code path in case the subtraction would underflow that simply assigns 0 as the lastClaim value.

COM-05M: Impermanent Migration Procedure

TypeSeverityLocation
Logical FaultMinorCommunity.sol:L869-L883

Description:

The managerJoinFromMigrated function should clear the manager from the _previousCommunity as otherwise, the manager may be removed from the current contract but claim back manager-ship by re-invoking this function.

Example:

contracts/community/Community.sol
869/**
870 * @notice Allows a manager from the previousCommunity to join in this community
871 */
872function managerJoinFromMigrated() external override eligibleManager(msg.sender) {
873 require(
874 IAccessControlUpgradeable(address(_previousCommunity)).hasRole(
875 MANAGER_ROLE,
876 msg.sender
877 ),
878 "Community::managerJoinFromMigrated: NOT_ALLOWED"
879 );
880 _setupRole(MANAGER_ROLE, msg.sender);
881
882 emit ManagerJoined(msg.sender);
883}

Recommendation:

We advise the manager to also revoke their role on the previous contract to ensure this action can only be performed once.

Alleviation:

The migration from the legacy implementation has been omitted from the codebase rendering this exhibit irrelevant.

COM-06M: 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/Community.sol
230/**
231 * @notice Used to initialize a new Community contract
232 *
233 * @param firstManager_ Community's first manager.
234 * Will be able to add others
235 * @param claimAmount_ Base amount to be claim by the beneficiary
236 * @param maxClaim_ Limit that a beneficiary can claim in total
237 * @param decreaseStep_ Value decreased from maxClaim each time a beneficiary is added
238 * @param baseInterval_ Base interval to start claiming
239 * @param incrementInterval_ Increment interval used in each claim
240 * @param previousCommunity_ Previous smart contract address of community
241 * @param minTranche_ Minimum amount that the community will receive when requesting funds
242 * @param maxTranche_ Maximum amount that the community will receive when requesting funds
243 * @param managerBlockList_ Addresses that have to not be managers
244 */
245function initialize(
246 address firstManager_,
247 uint256 claimAmount_,
248 uint256 maxClaim_,
249 uint256 decreaseStep_,
250 uint256 baseInterval_,
251 uint256 incrementInterval_,
252 uint256 minTranche_,
253 uint256 maxTranche_,
254 ICommunity previousCommunity_,
255 address[] memory managerBlockList_
256) external override initializer {
257 require(
258 baseInterval_ > incrementInterval_,
259 "Community::initialize: baseInterval must be greater than incrementInterval"
260 );
261 require(
262 maxClaim_ > claimAmount_,
263 "Community::initialize: maxClaim must be greater than claimAmount"
264 );
265
266 require(
267 minTranche_ <= maxTranche_,
268 "Community::initialize: minTranche should not be greater than maxTranche"
269 );
270
271 __AccessControl_init();
272 __Ownable_init();
273
274 _setupRole(MANAGER_ROLE, firstManager_);
275 _setRoleAdmin(MANAGER_ROLE, MANAGER_ROLE);
276
277 _claimAmount = claimAmount_;
278 _baseInterval = baseInterval_;
279 _incrementInterval = incrementInterval_;
280 _maxClaim = maxClaim_;
281 _minTranche = minTranche_;
282 _maxTranche = maxTranche_;
283 _previousCommunity = previousCommunity_;
284 _communityAdmin = ICommunityAdmin(msg.sender);
285 _decreaseStep = decreaseStep_;
286 _locked = false;
287
288 for (uint256 i = 0; i < managerBlockList_.length; i++) {
289 _managerBlockList.add(managerBlockList_[i]);
290 emit ManagerAddedToBlockList(managerBlockList_[i]);
291 }
292
293 transferOwnership(msg.sender);
294
295 emit ManagerAdded(msg.sender, firstManager_);
296}

Recommendation:

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

Alleviation:

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