veRWA - nonseodion's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L129-L143 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L152-L182

Vulnerability details

Impact

Anytime a user deposits during an epoch (i.e a week) he immediately is eligible to earn from the rewards for that lending market that week. This occurs because when the lending pool calls sync_ledger, LendingLedger adds the deposit to the users deposit for that week. It updates the lenders balance using these lines and the market's deposits here. It calculates the current epoch and uses it to update these balances.

This allows a user to deposit even 1 second before an epoch ends and still claim the rewards for that epoch.

Proof of Concept

       function testDepositAndClaimEndOfEpoch() public {
        uint248 amountPerEpoch = 1 ether;

        uint256 fromEpoch = WEEK * 5;
        uint256 toEpoch = WEEK * 10;

        address lendingMarket = vm.addr(5201314);

        vm.prank(goverance);
        ledger.whiteListLendingMarket(lendingMarket, true);
        payable(ledger).transfer(1000 ether);

        vm.prank(goverance);
        ledger.setRewards(fromEpoch, toEpoch, amountPerEpoch);

        // first attack 
        vm.warp(WEEK * 6 - 1); // 1 second before week 5 ends
        address lender = users[1];

        int256 delta = 1.1 ether;
        vm.prank(lendingMarket);
        ledger.sync_ledger(lender, delta); // user deposits in lending pool

        vm.prank(lender);
        vm.warp(WEEK * 6); // now in week 6
        uint256 balanceBefore = address(lender).balance;
        ledger.claim(lendingMarket, fromEpoch, fromEpoch);
        uint256 balanceAfter = address(lender).balance;
        assertTrue(balanceAfter - balanceBefore == 1 ether); // user claimed 1 ether

        vm.startPrank(lendingMarket);
        ledger.sync_ledger(lender, -delta); // user withdraws


        // second attack
        vm.warp(WEEK * 7 - 1); // 1 second before week 6 ends

        ledger.sync_ledger(lender, delta); // user deposits in lending pool
        vm.stopPrank();

        vm.prank(lender);
        vm.warp(WEEK * 7); // now in week 6
        balanceBefore = address(lender).balance;
        ledger.claim(lendingMarket, WEEK*6, WEEK*6); // user claims 1 ether
        balanceAfter = address(lender).balance;
        assertTrue(balanceAfter - balanceBefore == 1 ether); 

        vm.startPrank(lendingMarket);
        ledger.sync_ledger(lender, -delta); // user withdraws
    }

Tools Used

Foundry, Vscode

Recommended Mitigation Steps

Let the deposits during an epoch count towards the rewards earned in the next epoch.

 function sync_ledger(address _lender, int256 _delta) external {
        address lendingMarket = msg.sender;
        require(lendingMarketWhitelist[lendingMarket], "Market not whitelisted");

        _checkpoint_lender(lendingMarket, _lender, type(uint256).max);
        - uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
        + uint256 nextEpoch = ((block.timestamp + WEEK) / WEEK) * WEEK;
        int256 updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][nextEpoch]) + _delta;
        require(updatedLenderBalance >= 0, "Lender balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
        lendingMarketBalances[lendingMarket][_lender][nextEpoch] = uint256(updatedLenderBalance);

        _checkpoint_market(lendingMarket, type(uint256).max);
        int256 updatedMarketBalance = int256(lendingMarketTotalBalance[lendingMarket][nextEpoch]) + _delta;
        require(updatedMarketBalance >= 0, "Market balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
        lendingMarketTotalBalance[lendingMarket][nextEpoch] = uint256(updatedMarketBalance);
    }

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L356-L387 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L211-L278

Vulnerability details

Impact

Users should be able to have only one concurrent vote on a pool in GaugeController. When a user votes the weight of his vote is calculated using his _user_weight parameter and the slope and end time of his balance lock are used to calculate the bias for that pool.

But nothing stops him from delegating his balance to another voter who can then use it to vote in GaugeController. This means that many delegators can vote individually on a pool and also delegate their balance to another voter who uses all their balances to vote on the same pool. The time of delegation does not matter i.e before or after individual votes. This effectively allows them to have multiple votes on a pool. It can also be carried out by an individual user through multiple iterations of the following proof of concept.

Proof of Concept

Alice creates an attacker contract to:

1. Vote on a pool.
2. Delegate to another address of hers. The address can lock as low as 1*e-18 CANTO (assuming CANTO has 18 decimal places) to become a valid voter.
3. Vote using that address. This address can be a contract created cheaply using Openzeppelin's MinimalProxy, an implementation of EIP 1167.
4. Repeat from no. 2.

Tools Used

VsCode

Recommended Mitigation Steps

Depending on what the team actually wants, two different steps can be taken.

1. Set the slope to zero whenever a user delegates in VotingEscrow. This will ensure the vote weight of a wallet that delegates is zero.
2. Modify GaugeController to check the delegatee of a msg.sender by calling the locked function on VotingEscrow. GaugeController can revert if delegatee != msg.sender

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L383-L384 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L330-L331

Vulnerability details

Impact

Before a voter can withdraw, the end time of his locked balance must have passed and he must be the delegate for his balance.

For a voter who does not delegate and his lock time has passed, he can easily withdraw. But if the voter has delegated and the locked balance end time has passed, he satisfies the first condition but would fail the second condition because he is not the current delegatee. When he tries to redelegate, he is met with these conditions in the delegate function.

        require(toLocked.end > block.timestamp, "Delegatee lock expired");
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

He cannot redelegate back to himself without calling increaseAmount to increase the amount of CANTO he has deposited and resetting his lock time for another 5 years. Hence for a user that delegates to withdraw, he'll have to increase the amount he deposited, redelegate to himself and then wait for 5 years before he can withdraw his deposit.

Proof of Concept

1. Alice creates a lock and deposits 5 CANTO in the Voting Escrow.
2. Alice delegates to another user.
3. After 5 years, Alice decides to withdraw but cannot because she is not the current delegatee for her balance.
4. She cannot call delegate because her lock time is over so she calls increaseAmount which resets her lock time to another 5 years.
5. Alice delegates the balance back to herself.
6. After 5 years she comes back to withdraw her ~5 CANTO.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Allow users to be able to delegate back to themselves regardless of lock time.

-         require(toLocked.amount > 0, "Delegatee has no lock");
-         require(toLocked.end > block.timestamp, "Delegatee lock expired");
-         require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
---
+         if(_addr != msg.sender){
+             require(toLocked.amount > 0, "Delegatee has no lock"); // this check has already been done for msg.sender
+             require(toLocked.end > block.timestamp, "Delegatee lock expired");
+             require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
+         }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L384

Vulnerability details

Impact

A voter cannot redelegate his deposit except the new delegatee has a higher lock time than the current delegatee.

   require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

This is a good design choice for the benefit of the protocol. But for a user that wants to redelegate, undelegate he may be prevented by the current delegatee.

The current delegatee can achieve this by frontrunning any transaction that wants to redelegate an amount currently delegated to him. The frontrun transaction will increase his lock time, which will make the incoming transaction invalid. Although this can be avoided if the transaction is sent through a private RPC URL or if the new delegate's lock time can be increased and the delegation happens in the same block. Depending on the technical level of the voter, this might be feasible to perform.

Ultimately, the lock time of the new delegatee has to be increased to redelegate or undelegate which may not be favorable for him.

Proof of Concept

1. Alice creates a lock and deposits 5 CANTO in the Voting Escrow.
2. Alice delegates to another user.
3. Alice tries to redelegate but is frontrun by her current delegatee, which leaves her deposits stuck with the delegatee.

Tools Used

Vscode

Recommended Mitigation Steps

The current check seems more like a design choice to make sure voters remain in the contract longer than their lock time. Replacing it with the check below will help achieve the same objective with no detriment to the voter.

        require(toLocked.end >= locked_.end, "Cannot delegate to shorter lock");

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L223-L234

Vulnerability details

Impact

It has already been established that if the Voting Escrow is not interacted with by the user in 5 years, Voting Weight would be broken. This seems like what the protocol has already agreed on as a tradeoff because it's very unlikely for the contract not to be called at all after 5 years. Regardless, if there's a possible fix it should be applied to avoid this tradeoff completely.

The voter weight is affected because after 5 years the loop which runs for 255 times would not calculate lastPoint correctly. This makes the voter's weight to be applied to the wrong epoch (week). The voters will be able to withdraw but will not be able to vote using the VotingEscrow. If voting is still needed they would have to migrate to another contract. This can be fixed, check recommended mitigation steps.

Proof of Concept

1. Users deposit their funds.
2. Voting Escrow is not called for 5 years at least.
3. When user tries to delegate, withdraw, increaseAmount etc his voting Weight is applied to the wrong epoch here.

Tools Used

Vscode

Recommended Mitigation Steps

checkPoint can be called continuously, to make sure lastPoint is updated correctly. To prevent vote weights from being broken, we can modify the lines 223 to lines 234 to this:

        if (_addr != address(0)) {
            // If last point was in this block, the slope change has been applied already
            // But in such case we have 0 slope(s)
            require(lastPoint.blk == block.number, "lastPoint not completely updated");
            lastPoint.slope = lastPoint.slope + userNewPoint.slope - userOldPoint.slope;
            lastPoint.bias = lastPoint.bias + userNewPoint.bias - userOldPoint.bias;
            if (lastPoint.slope < 0) {
                lastPoint.slope = 0;
            }
            if (lastPoint.bias < 0) {
                lastPoint.bias = 0;
            }
        }

The fix comes with the tradeoff of allowing users only perform actions when lastPoint is up to date after 5 years. But this is easily remedied, since checkPoint can be called continuously by anyone to update lastPoint correctly.

Assessed type

Other