New undelegation logic is broken, it won't work if the currentDelegate is already the SPONSORSHIP_ADDRESS

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/main/src/TwabController.sol#L645-L651

Vulnerability details

Original Issue

H-06 - Resetting delegation will result in user funds being lost forever

Details

The previous implementation to reset the delegation was causing that the users lost their own delegate balance if they tried to reset the delegation to themselves by setting the _to parameter as the address(0), which that is the value used to determine if a user is the delegate or not.

• In a short summary, when the users delegated their delegate balance to another address, the default value for the delegated balance was updated to the new delegatee, and if the user wanted to reset the delegation to themselves, if they would _to as address(0), the underlying _transferDelegateBalance call will mistakenly move the delegated user funds to the 0 address.
  • At this point the user might try to call delegate again with their actual address, however now the (_to == _currentDelegate) check will be true and revert, because of the behaviour specified earlier. The user also can't delegate to any other address because they don't own their own delegate balance anymore. Their funds are officially lost forever.

Mitigation

The implemented mitigation is attempting to treat the value of the _to parameter to be used as indicative of an undelegation operation.

• If the _to parameter is the address(0), the delegate will be set to the SPONSORSHIP_ADDRESS, otherwise it will not be updated.

Conclusion of the Mitigation and Proof of Concept of the new Bug

• This mitigation introduces a new bug in the undelegation process.

  • The problem is that if the _currentDelegate is already the SPONSORSHIP_ADDRESS, and the user set _to as the address(0), the to address will be set also as the SPONSORSHIP_ADDRESS, this will cause the execution to be reverted because the to address is equals to the _currentDelegate.

• There are different ways how the _currentDelegate could end up being set to the SPONSORSHIP_ADDRESS, one of them is if the user calls the sponsor() in the Vault contract.

  • I coded a small PoC to demonstrate that this new undelegation logic will revert then the _currentDelegate is already set to the SPONSORSHIP_ADDRESS and the user wants to undelegate its delegate balance by passing the address(0) as the to parameter.

Coded PoC

• Add the below testFunction in the TwabController.t.sol file

function testFirstSponsorshipAndThenUndelegate() external {
  address _sponsorshipAddress = SPONSORSHIP_ADDRESS;

  assertEq(twabController.delegateOf(mockVault, alice), alice);

  vm.startPrank(mockVault);
  //@audit => Alice's funds in the mockVault are delegated to the sponsor
  twabController.sponsor(alice);

  assertEq(twabController.delegateOf(mockVault, alice), _sponsorshipAddress);

  uint96 _amount = 1000e18;
  twabController.mint(alice, _amount);

  assertEq(twabController.balanceOf(mockVault, alice), _amount);
  assertEq(twabController.delegateBalanceOf(mockVault, alice), 0);

  assertEq(twabController.balanceOf(mockVault, _sponsorshipAddress), 0);
  assertEq(twabController.delegateBalanceOf(mockVault, _sponsorshipAddress), 0);
  vm.stopPrank();

  //@audit => Now alice want to undelegate the SPONSORSHIP_ADDRESS using the new logic to perform undelegations
  vm.startPrank(alice);
  twabController.delegate(mockVault,address(0));
  vm.stopPrank();
}

• Run the PoC: forge test -m testFirstSponsorshipAndThenUndelegate -vvvv And this is the expected result, the tx to delegate to the address(0) should revert with the error message: "SameDelegateAlreadySet"

  
  [109875] TwabControllerTest::testFirstSponsorshipAndThenUndelegate()
  ├─ [2988] TwabController::delegateOf(0x0000000000000000000000000000000000001234, Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f]) [staticcall]
  │   └─ ← Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f]
  ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001234)
  │   └─ ← ()
  ├─ [31314] TwabController::sponsor(Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f])
  │   ├─ emit Delegated(vault: 0x0000000000000000000000000000000000001234, delegator: Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f], delegate: 0x0000000000000000000000000000000000000001)
  │   └─ ← ()
  ├─ [986] TwabController::delegateOf(0x0000000000000000000000000000000000001234, Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f]) [staticcall]
  │   └─ ← 0x0000000000000000000000000000000000000001
  ├─ [48656] TwabController::mint(Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f], 1000000000000000000000)
  │   ├─ emit IncreasedBalance(vault: 0x0000000000000000000000000000000000001234, user: Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f], amount: 1000000000000000000000, delegateAmount: 0)
  │   ├─ emit IncreasedTotalSupply(vault: 0x0000000000000000000000000000000000001234, amount: 1000000000000000000000, delegateAmount: 0)
  │   └─ ← ()
  ├─ [807] TwabController::balanceOf(0x0000000000000000000000000000000000001234, Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f]) [staticcall]
  │   └─ ← 1000000000000000000000
  ├─ [840] TwabController::delegateBalanceOf(0x0000000000000000000000000000000000001234, Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f]) [staticcall]
  │   └─ ← 0
  ├─ [2807] TwabController::balanceOf(0x0000000000000000000000000000000000001234, 0x0000000000000000000000000000000000000001) [staticcall]
  │   └─ ← 0
  ├─ [840] TwabController::delegateBalanceOf(0x0000000000000000000000000000000000001234, 0x0000000000000000000000000000000000000001) [staticcall]
  │   └─ ← 0
  ├─ [0] VM::stopPrank()
  │   └─ ← ()
  ├─ [0] VM::startPrank(Alice: [0x8CE502537D13f249834eAa02DDe4781EBFe0d40f])
  │   └─ ← ()
  ├─ [1056] TwabController::delegate(0x0000000000000000000000000000000000001234, 0x0000000000000000000000000000000000000000)
  │   └─ ← "SameDelegateAlreadySet(0x0000000000000000000000000000000000000001)"
  └─ ← "SameDelegateAlreadySet(0x0000000000000000000000000000000000000001)"

Test result: FAILED. 0 passed; 1 failed; finished in 2.36ms

Failing tests: Encountered 1 failing test in test/TwabController.t.sol:TwabControllerTest [FAIL. Reason: SameDelegateAlreadySet(0x0000000000000000000000000000000000000001)] testFirstSponsorshipAndThenUndelegate() (gas: 109875)

## Recommended Mitigation Steps
- The recommendation would be to set the `to` address to be the user's address instead of the SPONSORSHIP_ADDRESS, in this way the new undelegation process won't fail if the `_currentDelegate` has already been set to the SPONSORSHIP_ADDRESS.

```solidity
function _delegate(address _vault, address _from, address _to) internal {
  address _currentDelegate = _delegateOf(_vault, _from);
  ...

- address to = _to == address(0) ? SPONSORSHIP_ADDRESS : _to;
+ address to = _to == address(0) ? _from : _to;

  if (to == _currentDelegate) {
    revert SameDelegateAlreadySet(to);
  }

  ...
}

Assessed type

Context

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[djb15]

I think this report is incorrect no @asselstine?

It's intended behaviour to revert if you're trying to delegate to the same address that's already delegated to. In this case you're trying to delegate from SPONSORSHIP_ADDRESS to SPONSORSHIP_ADDRESS (because 0 is now arbitrarily mapped to SPONSORSHIP_ADDRESS).

The new implementation has changed the "reset" behaviour to specify the user address rather than address(0) to undelegate (i.e. delegate back to yourself).

[stalinMacias]

The new implementation has changed the "reset" behaviour to specify the user address rather than address(0) to undelegate (i.e. delegate back to yourself).

Actually, the new implementation attempted to undelegate by delegating to the SPONSORSHIP_ADDRESS if the address in the delegate() was passed as the 0x address. See the comment in the TwabController::_delegate()

[djb15]

Exactly my point! Why is forcing the delegation to the SPONSORSHIP_ADDRESS any different to forcing the delegation to the user address when calling with address(0)?

With your suggested change, If I had delegated to myself and then tried to delegate to address(0) then the transaction would also revert.

The sponsor has decided to map address(0) to the SPONSORSHIP_ADDRESS and there isn't anything invalid about that imo

[stalinMacias]

Because setting the current_delegate to the SPONSORSHIP_ADDRESS can be done not only by the new undelegation logic.

• Users using the Vault:sponsor() function will get their current_delegate to the SPONSORSHIP_ADDRESS, and then if they'd like to undelegate by passing the address(0), the undelegation will fail because current_delegate is already the SPONSORSHP_ADDRESS.

• If a user has delegated to themselves then there is no need to undelegate, their delegate_balance is already on their hands.

• But if a user has delegated to the sponsorship_address through the Vault:sponsor(), then there is required to undelegate and get their delegate_balance back on their hands

[asselstine]

Hmm the reproduction didn't work; this test failed as it should.

Need to dig in.

[stalinMacias]

@asselstine The test is expected to fail with the message "Reason: SameDelegateAlreadySet(0x0000000000000000000000000000000000000001)] "

[djb15]

Ok let me phrase this another way. Why does delegating to address(0) have to be considered "undelegating"? Why can't it be considered as "sponsoring"? I agree with you it's more logical to sponsor back to yourself, but just because it doesn't I don't think that's a bug.

The worst case scenario is:

1. A user has delegated to SPONSORSHIP_ADDRESS
2. A user tries to delegate to address(0) to reset their delegation but the transaction reverts because delegating to address(0) is the same as delegating to SPONSORSHIP_ADDRESS
3. The user then delegates to their address (which they could have done instead of step 2 anyway)

The end result is 1 reverted transaction with no funds lost, so surely QA at best?

I'm sure we could go round in circles with this anyway haha

[asselstine]

I believe I see the confusion: the implementation doesn't match the intent. It does work, however. It needs clarification!

I believe this is comment is the problem:

L646: // Treat address(0) as un-delegating

Because the comments also say:

L646: ... this lets
// them do so without knowing the sponsorship special value address.

The "this" is referring to address(0).

So in the same breath it's saying address(0) is used to un-delegate but is also used as an alias for the SPONSORSHIP_ADDRESS. So which is it?

I do like having address(0) interpreted as sponsorship. They are essentially burning their chance; this intuitive to me.

I also think removing the erroneous comment and adding some additional natspec on undelegating by delegating to yourself.

[stalinMacias]

okaay, so, looks like @asselstine has cleared all the doubts, so, I think you are right @djb15 , it could be a QA.

Thanks @asselstine for summarizing and explaining what was the intention of the address(0).

[Picodes]

Thanks for your explanations everyone. Will downgrade this to QA.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b