fix: avoid infinitely looping through forwarders when looking for a t…

…ransaction path (#285)

Fixes #235.

Turns out the transaction pathing algorithm is sufficiently smart, we just weren't handling the 0 case correctly (resulting in an infinite loop).

The reason we can't rely on permissions data once we start looking for forwarding path is because those aren't determined by permissions; we have to check `canForward()` for each step.

An additional optimization we could make is cache which forwarders can be linked together, but this seems premature since there's unlikely to be very large paths at this point.

packages/aragon-wrapper/src/index.js

@@ -1390,7 +1390,6 @@ export default class Aragon {
         return [directTransaction]
       }
 
-      // TODO: Support multiple needed roles?
       const roleSig = app.roles.find(
         (role) => role.id === method.roles[0]
       ).bytes
@@ -1402,7 +1401,7 @@ export default class Aragon {
         []
       )
 
-      // No one has access
+      // No one has access, so of course we (or the final forwarder) don't as well
       if (appsWithPermissionForMethod.length === 0) {
         return []
       }
@@ -1477,7 +1476,7 @@ export default class Aragon {
     // Check if one of the forwarders that has permission to perform an action
     // with `sig` on `address` can forward for us directly
     for (const forwarder of forwardersWithPermission) {
-      let script = encodeCallScript([directTransaction])
+      const script = encodeCallScript([directTransaction])
       if (await this.canForward(forwarder, sender, script)) {
         const transaction = createForwarderTransaction(forwarder, script)
         transaction.pretransaction = pretransaction
@@ -1507,19 +1506,21 @@ export default class Aragon {
       ]
     })
 
-    // Find the shortest path
+    // Find the shortest path via a depth-first search of forwarder paths
+    // We eagerly evaluate paths that look promising; as soon as we find a forwarder that
+    // "links" up with a previous forwarder, we continue exploring this path immediately.
     // TODO(onbjerg): Should we find and return multiple paths?
     do {
       const [path, [forwarder, ...nextQueue]] = queue.shift()
 
-      // Skip paths longer than 10
-      if (path.length > 10) continue
+      // Skip if no forwarder or the path is longer than 5
+      if (!forwarder || path.length > 5) continue
 
       // Get the previous forwarder address
       const previousForwarder = path[0].to
 
       // Encode the previous transaction into an EVM callscript
-      let script = encodeCallScript([path[0]])
+      const script = encodeCallScript([path[0]])
 
       if (await this.canForward(previousForwarder, forwarder, script)) {
         if (await this.canForward(forwarder, sender, script)) {
@@ -1534,8 +1535,13 @@ export default class Aragon {
           // The previous forwarder can forward a transaction for this forwarder,
           // but this forwarder can not forward for our address, so we add it as a
           // possible path in the queue for later exploration.
-          // TODO(onbjerg): Should `forwarders` be filtered to exclude forwarders in the path already?
-          queue.push([[createForwarderTransaction(forwarder, script), ...path], forwarders])
+          queue.push([
+            [createForwarderTransaction(forwarder, script), ...path],
+            // Avoid including the current forwarder as a candidate for the next step
+            // in the path. Note that this is naive and may result in repeating cycles,
+            // but the maximum path length would prevent against infinite loops
+            forwarders.filter((nextForwarder) => nextForwarder !== forwarder)
+          ])
         }
       }