Laksmana - Cache issue, user's funds are sent to the unintended ``urnFarm`` and ``voteDelegate``.

[sherlock-admin2]

Laksmana

Medium

Cache issue, user's funds are sent to the unintended urnFarm and voteDelegate.

Summary

Missing resetting of urnFarm and voteDelegate addresses after doing free.

Causing users' funds to be sent to unwanted urnFarm and voteDelegate addresses. when they want to lock again after doing free, but not to lock or stake on the previous urnFarm and voteDelegate.

Root Cause

Look at this function _free:

function _free(address urn, uint256 wad, uint256 fee_) internal returns (uint256 freed) {
        require(wad <= uint256(type(int256).max), "LockstakeEngine/overflow");
        address urnFarm = urnFarms[urn];
        if (urnFarm != address(0)) {
=>            LockstakeUrn(urn).withdraw(urnFarm, wad);
        }
        lsmkr.burn(urn, wad);
        vat.frob(ilk, urn, urn, address(0), -int256(wad), 0);
        vat.slip(ilk, urn, -int256(wad));
        address voteDelegate = urnVoteDelegates[urn];
        if (voteDelegate != address(0)) {
=>            VoteDelegateLike(voteDelegate).free(wad);
        }
        uint256 burn = wad * fee_ / WAD;
        if (burn > 0) {
            mkr.burn(address(this), burn);
        }
        unchecked { freed = wad - burn; } // burn <= wad always
    }

Look at the code, there is no resetting urnFarm and voteDelegate to address(0) after "withdraw" and "free".

So, when the user does free when urnFarm and voteDelegate are not at address(0) / where the user has funds in urnFarm and voteDelegate, it will "withdraw" and "free" the user's funds in urnFarm and voteDelegate, but after that it is not reset to address(0).

Then, when the user does lock again. It will send user funds to the previous urnFarm and voteDelegate addresses without user permission.

Check this out _lock:

function _lock(address urn, uint256 wad, uint16 ref) internal {
        require(urnOwners[urn] != address(0), "LockstakeEngine/invalid-urn");
        require(wad <= uint256(type(int256).max), "LockstakeEngine/overflow");
        address voteDelegate = urnVoteDelegates[urn];
        if (voteDelegate != address(0)) {
            mkr.approve(voteDelegate, wad);
=>            VoteDelegateLike(voteDelegate).lock(wad);
        }
        vat.slip(ilk, urn, int256(wad));
        vat.frob(ilk, urn, urn, address(0), int256(wad), 0);
        lsmkr.mint(urn, wad);
        address urnFarm = urnFarms[urn];
        if (urnFarm != address(0)) {
            require(farms[urnFarm] == FarmStatus.ACTIVE, "LockstakeEngine/farm-deleted");
=>            LockstakeUrn(urn).stake(urnFarm, wad, ref);
        }
    }

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

• User funds are sent to unwanted urnFarm and voteDelegate addresses.

• Although the user can change this after or before locking again, this actually violates the user's policy and wishes.

• Before "lock" again; the user calls selectVoteDelegate and selectFarm first to address zero, then calls selectVoteDelegate and selectFarm again to the desired address. This consumes a lot of effort and time.

• After "lock" again; the user calls selectVoteDelegate and selectFarm to the desired address. But if he does not want previous funds to be sent to the unwanted urnFarm and voteDelegate addresses for some reason. So, this violates the user policy.

PoC

• paste the code below into LockstakeEngine.t.sol

• run with forge test -vv --match-test test_lock_again_after_free

  function test_lock_again_after_free() public {
      address andi = makeAddr("andi");
      deal(address(mkr), andi, 100_000 * 10 ** 18);
      vm.startPrank(andi);
      mkr.approve(address(engine), 100_000 * 10 ** 18);
  
      address urnAndi = engine.open(0);
      address voteDelegate_andi = voteDelegateFactory.create();
      engine.lock(urnAndi, 100_000 * 10 ** 18, 5);
      // lock to voteDelegate_andi
      engine.selectVoteDelegate(urnAndi, voteDelegate_andi);
      assertEq(mkr.balanceOf(voteDelegate_andi), 100_000 * 10 ** 18);
  
      engine.free(urnAndi, andi, 100_000 * 10 ** 18);
      assertEq(mkr.balanceOf(voteDelegate_andi), 0);
      console.log("andi's fund in old voteDelegate after free:", mkr.balanceOf(voteDelegate_andi));
  
      deal(address(mkr), andi, 100_000 * 10 ** 18);
      mkr.approve(address(engine), 100_000 * 10 ** 18);
      engine.lock(urnAndi, 100_000 * 10 ** 18, 5);
      assertEq(mkr.balanceOf(voteDelegate_andi), 100_000 * 10 ** 18);
      console.log("andi lock again, but the funds sent were to the old voteDelegate:", mkr.balanceOf(voteDelegate_andi));
  }

Mitigation

• Reset urnFarm and voteDelegate addresses after free

  
  function _free(address urn, uint256 wad, uint256 fee_) internal returns (uint256 freed) {
      require(wad <= uint256(type(int256).max), "LockstakeEngine/overflow");
      address urnFarm = urnFarms[urn];
      if (urnFarm != address(0)) {

• LockstakeUrn(urn).withdraw(urnFarm, wad); // remove this code
• _selectFarm(urn, wad, urnFarm, address(0), 0); } lsmkr.burn(urn, wad); vat.frob(ilk, urn, urn, address(0), -int256(wad), 0); vat.slip(ilk, urn, -int256(wad)); address voteDelegate = urnVoteDelegates[urn]; if (voteDelegate != address(0)) {
• VoteDelegateLike(voteDelegate).free(wad); // remove this code
• selectVoteDelegate(urn, wad, voteDelegate, address(0)); } uint256 burn = wad * fee / WAD; if (burn > 0) { mkr.burn(address(this), burn); } unchecked { freed = wad - burn; } // burn <= wad always }

[telome]

Not an issue. Intended behaviour.