Elastic-Finance-DAO / eefi_contracts

0 stars 0 forks source link

BUG - AmplesenseVault #22

Closed pldespaigne closed 3 years ago

pldespaigne commented 3 years ago

This PR fix 4 bugs in the AmplesenseVault.sol:

  1. The contract was lacking a fallback function. This function was needed in order for the vault to receive eth (e.g. the trader try to send eth to the vault).

  2. The constructor had a nasty bug :

    constructor(IERC20 ampl_token)
    AMPLRebaser(ampl_token)
    Ownable() {
        rewards_eefi = new Distribute(9, IERC20(eefi_token)); // <--- eefi_token is not yet initialized, so address 0 is used, and the Distribute contract will think it should reward eth instead of a token
        rewards_eth = new Distribute(9, IERC20(0));
        eefi_token = new EEFIToken();
    }
  3. In the _rebase function (around line 171) the calculation of surplus seems strange and often returns 0 if new_balance is not high enough. I change the calculation, but I'm really not sure of what I've done, it would be better if you have a look.

    
    // ! WARNING CALCULATION OF SURPLUS (commented line) SEEMS STRANGE: allways get 0
    // uint256 surplus = new_supply.sub(old_supply).mul(new_balance).divDown(new_supply);

uint256 surplus = new_supply.sub(old_supply); // <- this is what I use, but I'm not sure


4. In the `_rebase` function _(around line 182)_ we transfer some ampl to the trader, but this will not work with current implementation of the `Mocktrader.sol`. Indeed the trader will itself try to `transferFrom` from the vault to itself, but it doesn't have the `allowance`

```Solidity
// _ampl_token.safeTransfer(address(trader), for_eefi.add(for_eth));
_ampl_token.approve(address(trader), for_eefi.add(for_eth)); // <- to fix the bug we remove transfer (as the trader already handle that) and increase allowance instead

Trader related code is in MockTrader.sol in sellAMPLForEth & sellAMPLForEEFI at line 32 & 42