Omniscia rain protocol Audit
Verify Manual Review Findings
Verify Manual Review Findings
VER-01M: Introduction of Single Point of Failure
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | Verify.sol:L222, L227, L232 |
Description:
The assignment of the X_ADMIN roles to themselves as being the role administrators permits a single X_ADMIN to remove all others and arbitrarily set the X set to their wish in a single transaction.
Example:
213/// Defines RBAC logic for each role under Open Zeppelin.214/// @param admin_ The address to ASSIGN ALL ADMIN ROLES to initially. This215/// address is free and encouraged to delegate fine grained permissions to216/// many other sub-admin addresses, then revoke it's own "root" access.217function initialize(address admin_) external initializer {218 require(admin_ != address(0), "0_ACCOUNT");219
220 // `APPROVER_ADMIN` can admin each other in addition to221 // `APPROVER` addresses underneath.222 _setRoleAdmin(APPROVER_ADMIN, APPROVER_ADMIN);223 _setRoleAdmin(APPROVER, APPROVER_ADMIN);224
225 // `REMOVER_ADMIN` can admin each other in addition to226 // `REMOVER` addresses underneath.227 _setRoleAdmin(REMOVER_ADMIN, REMOVER_ADMIN);228 _setRoleAdmin(REMOVER, REMOVER_ADMIN);229
230 // `BANNER_ADMIN` can admin each other in addition to231 // `BANNER` addresses underneath.232 _setRoleAdmin(BANNER_ADMIN, BANNER_ADMIN);233 _setRoleAdmin(BANNER, BANNER_ADMIN);234
235 // It is STRONGLY RECOMMENDED that the `admin_` delegates specific236 // admin roles then revokes the `DEFAULT_ADMIN_ROLE` and the `X_ADMIN`237 // roles.238 _setupRole(APPROVER_ADMIN, admin_);239 _setupRole(REMOVER_ADMIN, admin_);240 _setupRole(BANNER_ADMIN, admin_);241}Recommendation:
We advise the X_ADMIN role to be managed by a supervisor role that temporarily exists and is renounced from the system before the system itself is activated.
Alleviation:
After extensive discussion with the Rain Protocol team, we concluded that the needs of their protocol demand fluid role assignment and since our proposed solution relies on a supervisor role that is consequently renounced, we consider the exhibit nullified.
VER-02M: Potentially Dangerous Race Condition
| Type | Severity | Location |
|---|---|---|
| Language Specific | | Verify.sol:L297, L312 |
Description:
The add function accepts a data_ argument meant to represent proof that the caller is associated with an off-chain entity. Given that there is a delay between a transaction's execution and a transaction's submission to the network, it is possible for another address to submit the same set of evidence ahead of the original "true" submitter.
Example:
294/// An account adds their own verification evidence.295/// Internally `msg.sender` is used; delegated `add` is not supported.296/// @param data_ The evidence to support approving the `msg.sender`.297function add(bytes calldata data_) external {298 // Accounts may NOT change their application to be approved.299 // This restriction is the main reason delegated add is not supported300 // as it would lead to griefing.301 // A mistaken add requires an appeal to a REMOVER to restart the302 // process OR a new `msg.sender` (i.e. different wallet address).303 // The awkward < 1 here is to silence slither complaining about304 // equality checks against `0`. The intent is to ensure that305 // `addedSince` is not already set before we set it.306 require(states[msg.sender].addedSince < 1, "PRIOR_ADD");307 states[msg.sender] = State(308 uint32(block.number),309 UNINITIALIZED,310 UNINITIALIZED311 );312 emit RequestApprove(msg.sender, data_);313}Recommendation:
We advise this trait of the system to be carefully evaluated and its caveats to be listed in the documentation. At the best case scenario, it causes ambiguity in case the data_ proof is not definitive and at the worst case scenario it can cause malicious addresses to be approved due to human error as i.e. it is relatively easy to clone an Ethereum's address first and last alphanumeric characters, the most frequently checked characters when comparing addresses, and thus cause a social engineering attack vector on the contract. A potential commit-and-reveal scheme coupled with a cooldown mechanism can resolve this issue.
Alleviation:
We discussed with the Rain Protocol team and concluded that while the commit and reveal approach we advised is being experimented on, the current solution in place is adequate based on the comments of the function and the intended use of the data_ payload. As a result, we consider the exhibit nullified.