Remediate VKL-01S

[hswopeams]

The remediation is different from what is suggested in the audit, but it makes the code more legible. The complainPeriod and cancelFaultPeriod can't be constants (as suggested by the auditors) because they need to be settable using the setComplainPeriod and setCancelFaultPeriod functions. These functions are needed so that the periods can be set to lower values for testing on testnet. Otherwise, we would have to wait two weeks to test some scenarios.

As discussed, the set functions are now being called from the constructor. The set functions emit the events. Normally when calling a function from within the same contract, the function visibility is public. It is also necessary to change the set function visibility to public because it's not possible to call an external function on the same contract from the constructor.I made the function visibility change, but this necessitated removing the functions from the IVoucherKernel interface. I ran the gas reporter in the before and after situation by executing two test cases -- one that tests setComplainPeriod and one that calls setCancelPeriod. Both test cases cause the constructor to be called because the contracts are redeployed before each test case.

The result is that after this change, the gas used by the constructor goes UP by 4868. I've attached the gas reports. The GasReport-external-setters-not-called-from-constructor.txt represents the original situation before the change. Based on the gas reports and the fact that the issue pointed out by the auditors is to optimize gas, I'm not sure it is worth making this change. On the other hand, the constructor code is simplified.

GasReport-external-setters-not-called-from-constructor.txt GasReport-public-setters-called-from-constructor.txt

[zajck]

What if don't actually declare local variables and just set:

setComplainPeriod(7 * 1 days);
setCancelFaultPeriod(7 * 1 days);

That would save some gas. But yeah, overall it will still be more expensive.

Or alternatively make a contract level constant

uint256 internal constant WEEK = 7 * 1 days;

and then

setComplainPeriod(WEEK);
setCancelFaultPeriod(WEEK);

This should have comparable cost to my other suggestion.

In any case, I think that simplicity is better than gas cost optimization since it's one time cost. I'm supporting the change.

[hswopeams]

I added the WEEK constant, as suggested by @zajck. It saves a very small amount of gas (16) but also makes the code cleaner. GasReport-public-setters-called-from-constructor-with-constant.txt