ampleforth/ampleforth-contracts

Smart contracts for Ampleforth Protocol (working name uFragments)

ampleforth-contracts's Issues

`scaledTotalSupply` can be pure

Add a transferAll function

support for EIP-2612

https://eips.ethereum.org/EIPS/eip-2612

lib/Select: Average computation can overflow

The average computation in the Select.sol library can overflow because the average is computed with (a + b) / 2 (see here).
An overflow would be caught by Open Zeppelin's SafeMath library, but this would lead to reverting the transaction.

Even though the real values in the Ampleforth oracles are not big enough for overflows, consider distributing the computation with (a / 2) + (b / 2) + (((a % 2) + (b % 2)) / 2) to avoid the possibility altogether and improve the re-use of the Select.sol library.

SupplyChanges Threshold

Currently the supply change is only applied when the delta goes beyond 5% of the current supply. This can lead to short bursty and erratic changes in the supply since its highly unlikely that the price is going to change my 5% in a single day. On top of this we would only apply a fraction of the correction and while simulations show that the currency still remains pegged to the base price the supply changes are applied very unevenly.

And I think this boils down to setting the right expectations for the currency like do we want to folks buying our currency to have a timed schedule to supply changes or should we give them stability of the amount they have in their wallet.

I am attaching the screenshot from the simulation where the first one makes changes to the supply irrespective of the threshold and the second one would make changes only when the supply delta reaches 5%.

Expose scaled balances similar to aTokens

-> https://docs.aave.com/developers/the-core-protocol/atokens#scaledbalanceof
-> https://docs.aave.com/developers/the-core-protocol/atokens#scaledtotalsupply

Fix flaky time dependent tests

Time dependent tests for the last rebase timestamp are flaky

https://travis-ci.com/github/ampleforth/uFragments/builds/210413678

Orchestrator V2

Contributors can pick up of the issues listed here (all are good first issues, great to get started):

• The Orchestrator was created to disregard failed transactions. However it was patched to revert if one of the listed transactions fail (to prevent a gas underprice attack). Update the inline code documentation to reflect the patched change].

• Replace externalCall with .call

• The orchestrator imports the entire UFragmentsPolicy.sol. The typical way to address this is to just import the relevant interface, instead of importing the full contract. This helps reduce the deployed contract size.

• Use the revert message to indicate the index of the external call transaction which failed

• Use non upgradable version of Ownable in The Orchestrator contract.

• The current Orchestrator contract uses require(msg.sender == tx.origin) to prevent contracts from calling rebase. But tx.origin may be deprecated in the future. Implement one of these alternate mechanisms described here.

ERC-20 use external instead of public

Testnet contracts in rinkeby are defunct

Group audit

UFragments.sol

• Check the gas cost for require(address(this) != to) in validReceipt modifier.

• setMonetaryPolicy should NOT be callable only once.

• supplyDelta.abs().toUInt256Safe() to uint256(supplyDelta.abs()).

• Handle supplyDelta == 0 condition on rebase?

• Set initializer version number.

• Remove require in transferFrom require(value <= _allowedFragments[from][msg.sender]);. Let safemath fail if this condition is violated.

• This contract Should not be payable (No one should be able to send eth to this contract).

• Return new supply on rebase.

• Investiage the behvior of the fallback function.

UFragmentsPolicy.sol

• Rename appliedSupplyAdjustment to requestedSupplyAdjustment.

• Use uint256 for rebaseLag.

• Set initializer version number.

• @truncs to revist the deviation threshold simulation.

• Update the comment explaining deviation threshold to reflect that it's deviation from the target

• Initialize lastRebaseTimestamp = 0.

parse mock events helpers

Contacts

deploy.ts is broken

Several contract deployments fail:

An unexpected error occurred:
Error: missing argument: passed to contract (count=3, expectedCount=4, code=MISSING_ARGUMENT, version=contracts/5.7.0)

An unexpected error occurred:
Error: no matching function (argument="signature", value="initialize(address,address,uint256)", code=INVALID_ARGUMENT, version=abi/5.7.0)

e.g. MedianOracle

    function init(
        uint256 reportExpirationTimeSec_,
        uint256 reportDelaySec_,
        uint256 minimumProviders_,
        uint256 scalar_
    ) public initializer {
        require(reportExpirationTimeSec_ <= MAX_REPORT_EXPIRATION_TIME);
        require(minimumProviders_ > 0);
        reportExpirationTimeSec = reportExpirationTimeSec_;
        reportDelaySec = reportDelaySec_;
        minimumProviders = minimumProviders_;
        scalar = scalar_;
        __Ownable_init();
    }

in deploy.ts

    // deploy cpi oracle
    const cpiOracle = await deployContract(hre, 'MedianOracle', deployer)
    await cpiOracle.init(
      CPI_REPORT_EXPIRATION_SEC,
      CPI_REPORT_DELAY_SEC,
      CPI_MIN_PROVIDERS,
    )
    console.log('CPI oracle to:', cpiOracle.address)

Add crytic

https://github.com/crytic/slither/wiki/Upgradeability-Checks

What happens if there is a rebase in the same block as a transaction?

If I send 3 ampl twice which lets say represents 0.3% of the total supply (so I am sending 0.6%). These transactions happen to be in the same block with a rebase sandwiched between them.

What do the logs show?

3 ampl in both transactions? Or 3 ampl and 3.1 ampl?

setMonetaryPolicy callable only once

I think we force the setMonetaryPolicy method to be callable only ONCE after deployment.
The purpose of this function is to point the uFragments contract to the policy. Once that's done, we should never use this again. If we find a bug in the policy, we should rely on upgradability.

/**
   * @param monetaryPolicy_ The address of the monetary policy contract to use for authz.
   */
function setMonetaryPolicy(address monetaryPolicy_) external onlyOwner {
  monetaryPolicy = monetaryPolicy_;
}

https://github.com/frgprotocol/uFragments/blob/master/contracts/UFragments.sol#L93

@brandoniles @ahnaguib @truncs

Hardhat typescript compilation Errors

After cloning master branch and installing with yarn I get the following errors when trying to run yarn test

scripts/deploy.ts:32:69 - error TS2345: Argument of type 'SignerWithAddress' is not assignable to parameter of type 'Signer'.
Property 'getFeeData' is missing in type 'SignerWithAddress' but required in type 'Signer'.

32 (await hre.ethers.getContractFactory('UFragments')).connect(deployer),
~~~~~~~~

node_modules/@ethersproject/abstract-signer/lib/index.d.ts:38:5
38 getFeeData(): Promise;
~~~~~~~~~~
'getFeeData' is declared here.
scripts/deploy.ts:45:11 - error TS2345: Argument of type 'SignerWithAddress' is not assignable to parameter of type 'Signer'.

45 deployer,
~~~~~~~~
scripts/deploy.ts:59:16 - error TS2345: Argument of type 'SignerWithAddress' is not assignable to parameter of type 'Signer'.

59 .connect(deployer)
~~~~~~~~
scripts/deploy.ts:100:9 - error TS2345: Argument of type 'SignerWithAddress' is not assignable to parameter of type 'Signer | Provider | undefined'.
Type 'SignerWithAddress' is not assignable to type 'Signer'.

100 deployer,
~~~~~~~~
scripts/deploy.ts:112:9 - error TS2345: Argument of type 'SignerWithAddress' is not assignable to parameter of type 'Signer | Provider | undefined'.
Type 'SignerWithAddress' is not assignable to type 'Signer'.

112 deployer,
~~~~~~~~
scripts/deploy.ts:149:16 - error TS2345: Argument of type 'SignerWithAddress' is not assignable to parameter of type 'Signer'.

149 .connect(deployer)
~~~~~

Highly Critical Exploit (rebase)

I have an exploit to AMPLEFORTH uFragment/rebase.

Where can I submit this for bounty.

Warning: If this message gets ignored. I will post the exploit in PUBLIC within 24hours.

Accidental double spending on approve adjustment.

The issue mentioned in the comments here: https://github.com/frgprotocol/uFragments/blob/f2ee6d349076126bf4fd13f84538d309a3549f0f/contracts/UFragments.sol#L248

This issue/ambiguity is a general problem in ERC20 interface.

The common solution that I found mentioned or implemented by other ERC20 tokens is
to let the caller handle it by doing one transaction to approve(0) wait until it is mined, check how much was transferred before setting to zero and then do another approve with (new value - transferred amount)

README whitepaper link broken

add getter on policy which returns epoch and global supply