Certik Audit Recommendations (#151)

* Exhibit 1

* Exhibit 2

* Exhibit 5

* Exhibit 6

* Exhibit 7

* Exhibit 9

Co-authored-by: Brandon Iles <brandon@ampleforth.org>

Author: brandoniles

contracts/Orchestrator.sol

@@ -13,9 +13,9 @@ import "./UFragmentsPolicy.sol";
 contract Orchestrator is Ownable {
 
     struct Transaction {
+        bool enabled;
         address destination;
         bytes data;
-        bool enabled;
     }
 
     event TransactionFailed(address indexed destination, uint index, bytes data);
@@ -52,7 +52,7 @@ contract Orchestrator is Ownable {
             Transaction storage t = transactions[i];
             if (t.enabled) {
                 bool result =
-                    externalCall(t.destination, 0, t.data.length, t.data);
+                    externalCall(t.destination, t.data);
                 if (!result) {
                     emit TransactionFailed(t.destination, i, t.data);
                 }
@@ -70,9 +70,9 @@ contract Orchestrator is Ownable {
         onlyOwner
     {
         transactions.push(Transaction({
+            enabled: true,
             destination: destination,
-            data: data,
-            enabled: true
+            data: data
         }));
     }
 
@@ -90,7 +90,6 @@ contract Orchestrator is Ownable {
             transactions[index] = transactions[transactions.length - 1];
         }
 
-        delete transactions[transactions.length - 1];
         transactions.length--;
     }
 
@@ -109,7 +108,7 @@ contract Orchestrator is Ownable {
     /**
      * @return Number of transactions, both enabled and disabled, in transactions list.
      */
-    function transactionsLength()
+    function transactionsSize()
         external
         view
         returns (uint256)
@@ -120,12 +119,10 @@ contract Orchestrator is Ownable {
     /**
      * @dev wrapper to call the encoded transactions on downstream consumers.
      * @param destination Address of destination contract.
-     * @param transferValueWei ETH value to send, in wei.
-     * @param dataLength Size of data param.
      * @param data The encoded data payload.
      * @return True on success
      */
-    function externalCall(address destination, uint transferValueWei, uint dataLength, bytes data)
+    function externalCall(address destination, bytes data)
         internal
         returns (bool)
     {
@@ -147,9 +144,9 @@ contract Orchestrator is Ownable {
 
 
                 destination,
-                transferValueWei,
+                0, // transfer value in wei
                 dataAddress,
-                dataLength,  // Size of the input (in bytes). This is what fixes the padding problem
+                mload(data),  // Size of the input, in bytes. Stored in position 0 of the array.
                 outputAddress,
                 0  // Output is ignored, therefore the output size is zero
             )

contracts/UFragmentsPolicy.sol

@@ -83,7 +83,7 @@ contract UFragmentsPolicy is Ownable {
     uint256 private constant MAX_SUPPLY = ~(uint256(1) << 255) / MAX_RATE;
 
     // This module orchestrates the rebase execution and downstream notification.
-    address public orchestrator = address(0x0);
+    address public orchestrator;
 
     modifier onlyOrchestrator() {
         require(msg.sender == orchestrator);

test/unit/Orchestrator.js

@@ -61,7 +61,7 @@ contract('Orchestrator', function (accounts) {
     });
 
     it('should have no transactions', async function () {
-      (await orchestrator.transactionsLength.call()).should.be.bignumber.eq(0);
+      (await orchestrator.transactionsSize.call()).should.be.bignumber.eq(0);
     });
 
     it('should call rebase on policy', async function () {
@@ -84,7 +84,7 @@ contract('Orchestrator', function (accounts) {
     });
 
     it('should have 1 transaction', async function () {
-      (await orchestrator.transactionsLength.call()).should.be.bignumber.eq(1);
+      (await orchestrator.transactionsSize.call()).should.be.bignumber.eq(1);
     });
 
     it('should call rebase on policy', async function () {
@@ -120,7 +120,7 @@ contract('Orchestrator', function (accounts) {
     });
 
     it('should have 2 transactions', async function () {
-      (await orchestrator.transactionsLength.call()).should.be.bignumber.eq(2);
+      (await orchestrator.transactionsSize.call()).should.be.bignumber.eq(2);
     });
 
     it('should call rebase on policy', async function () {
@@ -168,7 +168,7 @@ contract('Orchestrator', function (accounts) {
     });
 
     it('should have 2 transactions', async function () {
-      (await orchestrator.transactionsLength.call()).should.be.bignumber.eq(2);
+      (await orchestrator.transactionsSize.call()).should.be.bignumber.eq(2);
     });
 
     it('should call rebase on policy', async function () {
@@ -203,7 +203,7 @@ contract('Orchestrator', function (accounts) {
     });
 
     it('should have 1 transaction', async function () {
-      (await orchestrator.transactionsLength.call()).should.be.bignumber.eq(1);
+      (await orchestrator.transactionsSize.call()).should.be.bignumber.eq(1);
     });
 
     it('should call rebase on policy', async function () {
@@ -238,7 +238,7 @@ contract('Orchestrator', function (accounts) {
     });
 
     it('should have 0 transactions', async function () {
-      (await orchestrator.transactionsLength.call()).should.be.bignumber.eq(0);
+      (await orchestrator.transactionsSize.call()).should.be.bignumber.eq(0);
     });
 
     it('should call rebase on policy', async function () {
@@ -267,7 +267,7 @@ contract('Orchestrator', function (accounts) {
     });
 
     it('should have 3 transactions', async function () {
-      (await orchestrator.transactionsLength.call()).should.be.bignumber.eq(3);
+      (await orchestrator.transactionsSize.call()).should.be.bignumber.eq(3);
     });
 
     it('should call rebase on policy', async function () {
@@ -337,7 +337,7 @@ contract('Orchestrator', function (accounts) {
 
     describe('setTransactionEnabled', async function () {
       it('should be callable by owner', async function () {
-        (await orchestrator.transactionsLength.call()).should.be.bignumber.gt(0);
+        (await orchestrator.transactionsSize.call()).should.be.bignumber.gt(0);
         expect(
           await chain.isEthException(
             orchestrator.setTransactionEnabled(0, true, {from: deployer})
@@ -346,7 +346,7 @@ contract('Orchestrator', function (accounts) {
       });
 
       it('should be not be callable by others', async function () {
-        (await orchestrator.transactionsLength.call()).should.be.bignumber.gt(0);
+        (await orchestrator.transactionsSize.call()).should.be.bignumber.gt(0);
         expect(
           await chain.isEthException(
             orchestrator.setTransactionEnabled(0, true, {from: user})
@@ -357,7 +357,7 @@ contract('Orchestrator', function (accounts) {
 
     describe('removeTransaction', async function () {
       it('should be not be callable by others', async function () {
-        (await orchestrator.transactionsLength.call()).should.be.bignumber.gt(0);
+        (await orchestrator.transactionsSize.call()).should.be.bignumber.gt(0);
         expect(
           await chain.isEthException(
             orchestrator.removeTransaction(0, {from: user})
@@ -366,7 +366,7 @@ contract('Orchestrator', function (accounts) {
       });
 
       it('should be callable by owner', async function () {
-        (await orchestrator.transactionsLength.call()).should.be.bignumber.gt(0);
+        (await orchestrator.transactionsSize.call()).should.be.bignumber.gt(0);
         expect(
           await chain.isEthException(
             orchestrator.removeTransaction(0, {from: deployer})