Internal audit feedback #1 - Nithin

[aalavandhan]

1. Did you reimplement open zeppelin's libraries because of an incompatible solidity version?
   If so can we just use the solidity version which is compatible with open zeppelin's latest release?
   https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/Access/Ownable.sol

2. Is there a standard/audited implementation for ERC-1271 which we can reference
   https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/Access/ERC1271.sol

3. I'm not really sure about the difference but, rather than using assembly to make the external call here we can just use to.value(value).gas(gas).call(data)?
   https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/ExternalCall/ExternalCall.sol#L55

4. "If the gas is 0 we assume that nearly all available gas can be used"
   https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/ExternalCall/ExternalCall.sol#L40
   Shouldn't the condition here be: gas == 0 ? gas : (gasleft() - 2500)
   Is there a test for the above condition? https://github.com/ampleforth/token-geyser-v2/blob/main/test/ExternalCall/ExternalCall.ts

5. return success? https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/ExternalCall/ExternalCall.sol#L46

6. Safemath? https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/UniversalVault.sol#L252

[thegostep]

• Did you reimplement open zeppelin's libraries because of an incompatible solidity version?

It's a workaround for this issue OpenZeppelin/openzeppelin-contracts#2402

Not as important now that we use different access control for vaults - will update to use OpenZeppelin. #119

• Is there a standard/audited implementation for ERC-1271 which we can reference

Here are some implementations in production:
https://github.com/argentlabs/argent-contracts/blob/9f1f8eddcfd26925174f5a34337f53145fdd9ffe/contracts/modules/TransferManager.sol#L487-L492
https://github.com/gnosis/safe-contracts/blob/94f9b9083790495f67b661bfa93b06dcba2d3949/contracts/GnosisSafe.sol#L316-L337
https://github.com/authereum/contracts/blob/903c49d35751b63768e3011216f740e4f509132a/contracts/account/ERC1271Account.sol#L31-L41

Here is the OpenZeppelin implementation:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4d5f696bb19cdd4ef52721147891d81491e9613f/contracts/utils/SignatureChecker.sol

• I'm not really sure about the difference but, rather than using assembly to make the external call here we can just use to.value(value).gas(gas).call(data)?
• "If the gas is 0 we assume that nearly all available gas can be used"
• return success?

Ah - I actually deprecated that file but forgot to delete. Using the OpenZeppelin implementation instead:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/90ed1af972299070f51bf4665a85da56ac4d355e/contracts/utils/Address.sol#L114-L121

#118

• Safemath?

Don't think the nonce will get high enough :)

[thegostep]

2. Is there a standard/audited implementation for ERC-1271 which we can reference

swapped to using OpenZeppelin template in #120

[aalavandhan]

7. Redundant logic? https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/Geyser.sol#L211

function getRewardAvailable() public view returns (uint256) {
  return getFutureRewardAvailable(block.timestamp);
}

[aalavandhan]

8. Rather than hard coding the the rage_quit gas limit on the vault contract, each could geyser expose this number as part of its standard interface.

https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/UniversalVault.sol#L81

[aalavandhan]

9. Add comment that end needs to be greater than start?
   https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/Geyser.sol#L358

[aalavandhan]

10. getTotalStakeUnits is a little confusing. Would be great if the name indicated that it uses the current timestamp
    https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/Geyser.sol#L315

[aalavandhan]

11. calculateRewardAvailable / calculateRewardMulti are also a little confusing.
    Maybe:
    calculateRewardAvailable -> calculateUnlockedRewards ,
    calculateRewardMulti -> calculateRewardsFormStakes

or something like that ..

https://github.com/ampleforth/token-geyser-v2/blob/main/contracts/Geyser.sol#L418

[thegostep]

8. Rather than hard coding the the rage_quit gas limit on the vault contract, each could geyser expose this number as part of its standard interface.

I went back and forth on this - while it seems more convenient to pick up the gas at runtime it brings in concerns about it being changed on the fly by the geyser.

Scenario: Geyser gets hacked and attacker increases gas stipend to > block gas limit to make sure no one can ragequit

[thegostep]

9. Add comment that end needs to be greater than start?

I've avoided putting natspec comments on getters since they are usually more confusing than reading the code itself and seriously bloats the size of the file. I could mention the restrictions in inline comments though.

[aalavandhan]

1. Rather than hard coding the the rage_quit gas limit on the vault contract, each could geyser expose this number as part of its standard interface.

I went back and forth on this - while it seems more convenient to pick up the gas at runtime it brings in concerns about it being changed on the fly by the geyser.

Scenario: Geyser gets hacked and attacker increases gas stipend to > block gas limit to make sure no one can ragequit

Makes sense 👍

Maybe add a comment in the Geyser's rage quit function implementation about this gas limit restriction.