SwapperRegistry Manual Review Findings

SRY-01M: Ambiguous Role Management

Type	Severity	Location
Logical Fault		SwapperRegistry.sol:L60-L65, L68-L73, L76-L81, L84-L89

Description:

The contract contains a special Role notion that allows a particular address to either be NONE, a SWAPPER or a SUPPLEMENTARY_ALLOWANCE_TARGET, however, the functions that adjust those roles perform no validation of the previous role held by the address rendering ambiguity such as removeSwappersFromAllowlist being identical to removeSupplementaryAllowanceTargetsFromAllowlist in the end-result.

Example:

Copy
59/// @inheritdoc ISwapperRegistry
60function allowSwappers(address[] calldata _swappers) external onlyRole(ADMIN_ROLE) {
61  for (uint256 i; i < _swappers.length; i++) {
62    _accountRole[_swappers[i]] = Role.SWAPPER;
63  }
64  emit AllowedSwappers(_swappers);
65}
66
67/// @inheritdoc ISwapperRegistry
68function removeSwappersFromAllowlist(address[] calldata _swappers) external onlyRole(ADMIN_ROLE) {
69  for (uint256 i; i < _swappers.length; i++) {
70    _accountRole[_swappers[i]] = Role.NONE;
71  }
72  emit RemoveSwappersFromAllowlist(_swappers);
73}
74
75/// @inheritdoc ISwapperRegistry
76function allowSupplementaryAllowanceTargets(address[] calldata _allowanceTargets) external onlyRole(ADMIN_ROLE) {
77  for (uint256 i; i < _allowanceTargets.length; i++) {
78    _accountRole[_allowanceTargets[i]] = Role.SUPPLEMENTARY_ALLOWANCE_TARGET;
79  }
80  emit AllowedSupplementaryAllowanceTargets(_allowanceTargets);
81}
82
83/// @inheritdoc ISwapperRegistry
84function removeSupplementaryAllowanceTargetsFromAllowlist(address[] calldata _allowanceTargets) external onlyRole(ADMIN_ROLE) {
85  for (uint256 i; i < _allowanceTargets.length; i++) {
86    _accountRole[_allowanceTargets[i]] = Role.NONE;
87  }
88  emit RemovedAllowanceTargetsFromAllowlist(_allowanceTargets);
89}

Recommendation:

We advise some additional checks as to the previous state of each _accountRole to be introduced on each account maintenance function to ensure the contract emits the correct events for each operation and does not f.e. permit a direct downgrade of a SWAPPER role to a SUPPLEMENTARY_ALLOWANCE_TARGET without the relevant events and steps performed.

Alleviation:

All referenced functions have had appropriate checks ensuring that the account state transitions are from an empty state to a non-empty state and vice versa at all times and alleviating this exhibit.

SRY-02M: Ill-Advised Self-Ownership of Role

Type	Severity	Location
Code Style		SwapperRegistry.sol:L27

Description:

The SUPER_ADMIN_ROLE is owned by itself permitting the role to be arbitrarily assigned and a single assignee to be able to overtake the whole system by removing the role from other parties in the system.

Example:

Copy
26// We are setting the super admin role as its own admin so we can transfer it
27_setRoleAdmin(SUPER_ADMIN_ROLE, SUPER_ADMIN_ROLE);

Recommendation:

To prevent mis-use and generally increase the resilience of the system, we advise dedicated "transfer of ownership" workflows to be coded in the contract that make use of the internal functions exposed by the AccessControl implementation and to avoid assigning self-ownership of the role via the _setRoleAdmin function.

Alleviation:

The Mean Finance evaluated this exhibit and opted to retain the current behaviour in place citing a relevant issue of the original OpenZeppelin dependency as rationale that a manual transfer of a role would still be flawed in regards to the system being compromised. We would like to clarify that what we advised is that special "transfer of ownership" workflows are introduced to the contract which can in turn be time-delayed or guarded by other such security features that permit an incorrect / malicious transfer to be cancelled rather than be exploited immediately. In any case, we consider this exhibit acknowledged as the Mean Finance team has opted to retain the current paradigm across all contracts.