Omniscia Keyko Audit
Community Manual Review Findings
Community Manual Review Findings
COM-01M: Improper Role Management System
Type | Severity | Location |
---|---|---|
Standard Conformity | Medium | Community.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:
573/**574 * @notice Adds a new manager575 *576 * @param account_ address of the manager to be added577 */578function addManager(address account_)579 external580 override581 onlyManagersOrCommunityAdmin582 eligibleManager(account_)583{584 _setupRole(MANAGER_ROLE, account_);585 emit ManagerAdded(msg.sender, account_);586}587
588/**589 * @notice Remove an existing manager590 *591 * @param account_ address of the manager to be removed592 */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
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | Community.sol:L812-L827 |
Description:
The transfer
function allows the owner to transfer an arbitrary token out of the system.
Example:
812/**813 * @notice Transfers an amount of an ERC20 from this contract to an address814 *815 * @param token_ address of the ERC20 token816 * @param to_ address of the receiver817 * @param amount_ amount of the transaction818 */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
Type | Severity | Location |
---|---|---|
Input Sanitization | Medium | Community.sol:L248, L249, L509, L510, L909 |
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:
892/**893 * @notice Changes the state of a beneficiary894 *895 * @param beneficiary address of the beneficiary896 * @param newState_ new state897 */898function _changeBeneficiaryState(Beneficiary storage beneficiary, BeneficiaryState newState_)899 internal900{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
Type | Severity | Location |
---|---|---|
Mathematical Operations | Medium | Community.sol:L863 |
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:
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
Type | Severity | Location |
---|---|---|
Logical Fault | Minor | Community.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:
869/**870 * @notice Allows a manager from the previousCommunity to join in this community871 */872function managerJoinFromMigrated() external override eligibleManager(msg.sender) {873 require(874 IAccessControlUpgradeable(address(_previousCommunity)).hasRole(875 MANAGER_ROLE,876 msg.sender877 ),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
Type | Severity | Location |
---|---|---|
Standard Conformity | Minor | Community.sol:L25-L28, L271, L272 |
Description:
The re-entrancy guard contract is never initialized in the initialize
call chain, thus causing it to be improperly set up.
Example:
230/**231 * @notice Used to initialize a new Community contract232 *233 * @param firstManager_ Community's first manager.234 * Will be able to add others235 * @param claimAmount_ Base amount to be claim by the beneficiary236 * @param maxClaim_ Limit that a beneficiary can claim in total237 * @param decreaseStep_ Value decreased from maxClaim each time a beneficiary is added238 * @param baseInterval_ Base interval to start claiming239 * @param incrementInterval_ Increment interval used in each claim240 * @param previousCommunity_ Previous smart contract address of community241 * @param minTranche_ Minimum amount that the community will receive when requesting funds242 * @param maxTranche_ Maximum amount that the community will receive when requesting funds243 * @param managerBlockList_ Addresses that have to not be managers244 */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.