[Lines of code](https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L555-L625) # Vulnerability details ## Impact The contest documentation states: > Users may delegate ther lock to another user whereby they give the delegatee control over their lock expiration and balance (i.e. voting power). ... Moreover, the delegatee's lock expiration needs to be longer than the delegator's. It would be reasonable for a delegator to check that a potential delegatee has a lock later than their own. Attempts to delegate to a delegatee with a shorter lock than them are reverted. However, a Lock delegatee can still withdraw before a delegator's lock expires. This has the following impact on the delegator: 1. The delegator cannot withdraw delegated funds at the end of the lock period. 2. The delegator cannot undelegate. 3. The delegator cannot withdraw via [quitLock()]() because the lock expired. I identified two routes out of this scenario available to the user: 1. The delegator can extend their lock, undelegate the address, wait a week for the lock to expire and then withdraw. 2. The delegator can extend their lock, withdraw via [quitLock()]() with a penalty. The delegatee is not informed that they are a delegatee, and the delegator is not informed when the delegatee exits their lock. Talking to @Elniz, he believes that this is the correct approach. However, the user is forced into a position where they must spend money, then either wait or lose money (via [quitLock()]()) in order to recover their funds. A [forced undelegation]() option exists via the blocklist, but this currently only runs against contracts. This could form the basis of an exit route that doesn't require delegators to jump through additional hoops or delays to regain their tokens in the event of a delegatee's unscheduled departure. Regardless of whether this is a problem or a desired feature, the user must incur costs and penalties to recover funds under this scenario, or be denied access to funds for an extended period of time. I've submitted this as a medium because it appears to be an edge case in a desired feature but there is a substantial impact on delegator capital even when following instructions. If delegator and delegatee are isolated people this is just a bad experience for uninformed delegators. But if this were a delegatee connected to a project built on top of the vault (e.g. a bribe optimizer) they may attract a substantial proportion of delegators. If a popular delegator increased their lock end to the max 1 year from current timestamp then called [quitLock()]() that would lock affected accounts up for an additional year. If the delegatee suspected a proportion of delegators were not actively monitoring them, the delegatee could wait for their lock expiry, withdraw funds, re-lock with 1 FDT, call [increaseUnlockTime()]() with the maximum amount then call [quitLock()]() in one block. Delegators could call [quitLock()]() but penalties may be steep. There are DAO routes out of this scenario but would affect maths (e.g. calling [unlock()]() to set maxPenalty to 0). ## Proof of Concept Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. I created the following test demonstrating a bad actor locking delegation and it's impact. This should be added to test/votingEscrowTest.ts: describe.only("0xmatt - Bob pulls out early.", async () => { it("Alice and Bob lock FDT in ve. Alice delegates to Bob.", async () => { await createSnapshot(provider); const aliceLockTime = 4 * WEEK + (await getTimestamp()); const bobLockTime = 6 * WEEK + (await getTimestamp()); await ve.connect(alice).createLock(lockAmount, aliceLockTime); await ve.connect(bob).createLock(lockAmount, bobLockTime); // alice delegates await ve.connect(alice).delegate(bob.address); }); it("Bob pulls out early paying a penalty.", async () => { // Increase time to 2 weeks to lock end await increaseTimeTo((await ve.lockEnd(bob.address)).sub(WEEK * 3)); // Bob quits lock tx = ve.connect(bob).quitLock(); await expect(tx).not.to.be.reverted }); it("Alice waits till lock end, fails because delegated", async () => { // Increase time to alice's lock end - not needed if bobLockTime - 2w >= aliceLockTime await increaseTimeTo(await ve.lockEnd(alice.address)); tx = ve.connect(alice).withdraw(); await expect(tx).to.be.revertedWith("Lock delegated"); }); it("Alice now attempts to redelegate to herself. This fails because delegatee lock expired", async () => { // undelegate tx = ve.connect(alice).delegate(alice.address); await expect(tx).to.be.revertedWith("Delegatee lock expired") await restoreSnapshot(provider); }); }); ## Tools Used VSCode, Remix, HardHat ## Recommended Mitigation Steps The optimal solution is to check whether an account is a delegatee at the point of withdrawal. If they're a delegatee, return the delegated funds to delegators at this point. This would require a reverse mapping of delegatees => delegators[] or a walk of addresses with locks at withdrawal. It may also be possible to modify withdraw() to check if locked_[msg.sender].delegatee's lock has expired, and undelegate prior to processing the withdrawal. When someone is added to the blocklist [forceUndelegate()]() is invoked against that address. Blocked users are still able to withdraw at lock expiry - modifying withdraw to forceUndelegate() should have a similar effect. If this is too difficult, an alternative would be to allow users to invoke [VotingEscrow.sol#forceUndelegate()]() on themselves when they've delegated and their delegatee lock has expired. This is sub-optimal for the user but is at least a clear single user action. --- The text was updated successfully, but these errors were encountered: All reactions