Storytelling part

At the moment of publication Eco is using new contracts which are not impacted by both of vulnerabilities; fix is covered with this PR dated Dec 10, 2022.

2 bugs are covered within this article. I reported high (as I considered) bug last December, but project told they already fixed several things, including this issue, after another bug report, and their actual addresses are handling all of cases properly.

However, their Immunefi Bug Bounty page and GitHub repo were containing old contracts (they apologised for not updating them at proper time), and there was no obvious way to discover that they fixed something.

Looking back after several negotiations with various projects I understand that I was allowed to argue, stand on my position and probably make it a paid report. But that’s OK. I’m not blaming Eco here for anything, as - if I was part of their team - I would process that report in same manner.

On other hand, now I understand that I was able to argue and probably get rewarded on this report. Bug Bounty page docs is a public contract, and both sides must follow it.

I have reports that were able to do real harm to the projects but were considered invalid due to “out of scope” and I see that projects can be forced by Immunefi to follow what is written on Bug Bounty page, even if they do not want to.

My “Out of scope” reports with real impact were actually rewarded with smaller payout and my respects goes to those projects; it is represented in “total payouts” number but not in “report count” on leaderboard, which is a bit sad.

Also, there is really nice thing on some Immunefi projects now, called “primacy of impact”, which feels really like game-changer.

Lockup bug analysis part

Let’s start with a code. You can see an original code here:

I cleaned up and stylised code a little bit to show the code, not comments and notes.

pragma solidity ^0.8.0;

contract Lockup {
    struct DepositRecord {
        uint256 gonsDepositAmount;
        uint256 ecoDepositReward;
        uint256 lockupEnd;
        address delegate;
    }

    ERC20 public immutable ecoToken; //set up in constructor
    CurrencyTimer public immutable currencyTimer; //set up in constructor
    uint256 public duration; //set up in initializer
    uint256 public depositWindowEnd; //set up in initializer
    uint256 public constant DEPOSIT_WINDOW = 2 days;
    uint256 public interest; //set up in initializer
    uint256 public constant INTEREST_DIVISOR = 1e9;
    mapping(address => DepositRecord) public deposits;
    ...
    function deposit(uint256 _amount) external {
        internalDeposit(_amount, msg.sender, msg.sender);
    }

    function depositFor(uint256 _amount, address _benefactor) external {
        internalDeposit(_amount, msg.sender, _benefactor);
    }

    function withdraw() external {
        doWithdrawal(msg.sender, true);
    }

    function withdrawFor(address _who) external {
        doWithdrawal(_who, false);
    }

    function doWithdrawal(address _owner, bool _allowEarly) internal {
        DepositRecord storage _deposit = deposits[_owner];
        uint256 _gonsAmount = _deposit.gonsDepositAmount;
        require(_gonsAmount > 0);
        bool early = getTime() < _deposit.lockupEnd;
        require(_allowEarly || !early);
        uint256 _inflationMult = ecoToken.getPastLinearInflation(block.number);
        uint256 _amount = _gonsAmount / _inflationMult;
        uint256 _rawDelta = _deposit.ecoDepositReward;
        uint256 _delta = _amount > _rawDelta ? _rawDelta : _amount;
        _deposit.gonsDepositAmount = 0;
        _deposit.ecoDepositReward = 0;
        ecoToken.undelegateAmountFromAddress(_deposit.delegate, _gonsAmount);
        require(ecoToken.transfer(_owner, _amount));
        currencyTimer.lockupWithdrawal(_owner, _delta, early);
    }

    function internalDeposit(uint256 _amount, address _payer, address _who) private {
        require(getTime() < depositWindowEnd);
        require(ecoToken.transferFrom(_payer, address(this), _amount));
        DepositRecord storage _deposit = deposits[_who];
        uint256 _inflationMult = ecoToken.getPastLinearInflation(block.number);
        uint256 _gonsAmount = _amount * _inflationMult;
        address _primaryDelegate = ecoToken.getPrimaryDelegate(_who);
        ecoToken.delegateAmount(_primaryDelegate, _gonsAmount);
        _deposit.lockupEnd = getTime() + duration;
        _deposit.ecoDepositReward += (_amount * interest) / INTEREST_DIVISOR;
        _deposit.gonsDepositAmount += _gonsAmount;
        _deposit.delegate = _primaryDelegate;
    }
}

Can you see both of the bugs?

It is OK to not see both of them, as #1 is fully represented in this code, however, #2 requires to understand how delegation mechanics work.

If you want to try finding bug yourself, take a break and have a look on depositFor function, which is entry point of attack vector I’ve found. Another one is also abusing depositFor, but in way smarter manner.

Bug #1. lockupEnd extension

This is the thing I discovered by myself.

Functions that affect others are always interesting spot to have a look. Here, depositFor allows to do some manipulations with benefactor, which is nice.

So, let’s have a look how the state will be changed in internalDeposit:

• ECO will be forced to delegateAmount passed - the amount we just transferred few lines above, multiplied by getPastLinearInflation. Well, this looks OK, Lockup contract is just allowing you to delegate what you deposited, with some bonus for locking up your funds

• benefactors deposit record gets modified:

  • ecoDepositReward and gonsDepositAmount increases. Seems legit

  • delegate is changed to primary delegate of benefactor in ECO contract itself, which also seems legit (actually, it is not, but it is #2 topic)

  • lockupEnd is increased to now + duration, and this is the thing that actually looked suspicious for me

To sum up: contract says that we are able to deposit anything to anyone via depositFor(0, victim), which will increase victim’s lockupEnd to now + duration.

I would personally call this just griefing, as this duration in all real contracts of Eco is just 1 day, but according to Immunefi severity classification it is actually called Temporary freezing of funds. This is already classified as High, but what is a bit more interesting is that withdrawal function by default is applying flag allowEarly.

But things are actually more interesting! If we will look at currencyTimer.lockupWithdrawal function, you will see that early flag is passed as penalty flag, which means that by default user is able to withdraw anytime, but if he will do it before a lockup, he will pay instead of getting rewards, and his tokens will be burnt.

This means that attacker is able to use MEV to place his transaction with depositFor(0, victim) before victim calling withdrawal , which will make victim efficiently lose money instead of earning them, which can be classified by Immunefi as Permanent freezing of funds (well, it is loss, not freezing, but from user perspective it is more or less same), which is Critical.

Actually, I’m writing this lines and feel funny for myself for underrating my bug report as high 6 month ago.

In conclusion, this were my recommendations to the project based on this report:

1. remove flag _allowEarly: true from withdraw and introduce separate forceWithdraw so there will be clear difference between user's intentions

2. only allow depositFor for targets with no locks yet or introduce approval model OR split deposits so user

3. (optional) add check in internalDeposit that verifies that amount is greater than zero

Bug #2. locking the funds

This is the reason Eco updated their contract, efficiently fixing all quirks.

Nice description can be found in PR itself:

If the Lockup monetary policy was activated, an address could siphon voting power from other participants in the lockup and lock their funds forever. The attacker address would delegate to a benefactor address and deposit a large amount into the lockup. This would cause the lockup to delegate that large amount to the benefactor address. Then, the attacker address would change their delegate to a victim address with an existing amount in the lockup. When the attacker address deposits a small amount into the lockup, the lockup contract would delegate the small amount to the victim address, but then add that small amount to the initial deposit as the total deposit. When the attacker withdraws, the lockup contract undelegates the total deposit amount from the victim address. The end result is that the benefactor address keeps the voting power forever and the victim address is unable to withdraw from the lockup because the lockup no longer has enough delegated to that address to undelegate.

This is really hard to read, I had to do it 4 times to understand, let’s try to decompose and understand how it really works.

So, this is the simplified attack vector:

• Victim is depositing, let’s say, 100 ECO, and he does not possess any other ECO. Delegation is handled too, so now Victim is delegated from Lockup contract with 100 ECO.

• Attacker is depositing same amount of 100 ECO

• internalDeposit is doing delegation via ecoToken.delegateAmount(_primaryDelegate, _gonsAmount);

• Attacker changes his primaryDelegate to Victim, and then calls deposit again. Maybe even with amount 0, which, as we already know, is OK.This will, among other things, do _deposit.delegate = _primaryDelegate

• After that, he does withdrawal, which makes this call:ecoToken.undelegateAmountFromAddress(_deposit.delegate, _gonsAmount). As we remember, _deposit.delegate is Victim now, so after this call contract VoteCheckpoints will have this state: _delegates[msg.sender][delegatee] == 0.

• Then, when Victim will attempt to withdraw something, he will not be able to do so, as he will be failing on this check:

  require(
      _delegates[msg.sender][delegatee] >= amount,
      "amount not available to undelegate"
  );
  

Basically, this is also Permanent freezing of funds, which is Critical.

Sadly, I do not know who reported this bug to give my respects to him, and I do not know what were his recommendations, but I definitely think that it is excellent work - and a lesson for me.

I was looking through Lockup.sol for some time while building the report - and code surface is really small there - and did not note another critical laying literally on the next line to the one I was focused on.

Thanks for reading! Subscribe to my Twitter for more web3 security stuff: