veRWA - lanrebayode77's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L367-L375 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L397-L403

Vulnerability details

Impact

Voting with the same collateral multiple times by delegating and undelegating, a process that could manipulatively influence(increase) the weight of a particular lending market where the malicious actor is the major Liquidity provider.

Proof of Concept

The protocol allows depositors to delete to another address, but this feature comes with a problem:

1. It allows double voting with the same collateral, as no prevention for this as confirmed by the sponsor that no mitigation for yet https://drive.google.com/file/d/1_542keetegWF3idpF3bAqnNjxL9r9eUy/view?usp=sharing

2. In function createLock(uint256 _value) external payable nonReentrant { user creates a lock by depositing value, in return an amount of voting power in given, which enables the depositor to participate in voting for preferred lending market. According to the documentation.

Users can lock up CANTO (for five years) in the VotingEscrow contract to get veCANTO. They can then vote within GaugeController for different lending markets that are white-listed by governance.

Also, the protocols allows delegation of voting power to another address function delegate(address _addr) external nonReentrant {

 if (delegatee == msg.sender) {
            // Delegate
            fromLocked = locked_;
            toLocked = locked[_addr];

In VotingEscrow. _delegate, the new delegatee amount is updated to that of the delegator:

 if (action == LockAction.DELEGATE) {
            newLocked.delegated += value;
            emit Deposit(addr, uint256(int256(value)), newLocked.end, action, block.timestamp);
        

The implication of this is that, this new address now has the power to vote in GaugeController.vote_for_gauge_weights(), the problem with this is that, the real owner can later reclaim power by undelegating according to https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L385C9-L385C72 _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE); where value is now retrieved from delegatee and retained by the previous owner who now has same power to vote again for the same lending market. This means a malicious actor can switch address to vote for a lending market of choice(where he has major shares to get more REWARD).

The reason for this attack is that when delegating, only value changes

   if (action == LockAction.DELEGATE) {
            newLocked.delegated += value;
            emit Deposit(addr, uint256(int256(value)), newLocked.end, action, block.timestamp);
        } else {
            newLocked.delegated -= value;
            emit Withdraw(addr, uint256(int256(value)), action, block.timestamp);

Potential Attack Scenario:

1. Attacker A locks tokens and delegates voting power to address B.
2. Attacker A can now vote using address B's voting power.
3. Attacker A undelegates, but retains the voting power.
4. Now, Attacker A can vote again using their original address and the delegated address B, effectively 
double voting.

Tools Used

Manual review

Recommended Mitigation Steps

Implement checks within the delegation process to ensure that the same voting power cannot be delegated to two different addresses. This could involve maintaining a record of previously delegated votes for each user and preventing new delegations until undelegation or a specific cooldown period has passed.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/9a2e7be003bc1a77b3b87db31f3d5a1bcb48ed32/src/LendingLedger.sol#L158 https://github.com/code-423n4/2023-08-verwa/blob/9a2e7be003bc1a77b3b87db31f3d5a1bcb48ed32/src/LendingLedger.sol#L25-L26 https://github.com/code-423n4/2023-08-verwa/blob/9a2e7be003bc1a77b3b87db31f3d5a1bcb48ed32/src/LendingLedger.sol#L176 https://github.com/code-423n4/2023-08-verwa/blob/9a2e7be003bc1a77b3b87db31f3d5a1bcb48ed32/src/LendingLedger.sol#L159

Vulnerability details

Impact

First Time Claiming of Reward made impossible for Lenders.

Proof of Concept

When userClaimedEpoch mapping was created, it is meant to hold the last epoch time a lender claimed Reward.

  /// @dev Lending Market => Lender => Epoch
    mapping(address => mapping(address => uint256)) public userClaimedEpoch; // Until which epoch a user has claimed for a particular market (exclusive this value)

This value is usually set/updated after the Lender has claimed reward till a particular epoch, userClaimedEpoch[_market][lender] = claimEnd + WEEK;

However, before a Lender is able to claim Reward, it was required that userClaimedEpoch be greater than zero, require(userLastClaimed > 0, "No deposits for this user"); a condition that can NEVER be fulfilled for first time reward claimer(that is when claiming Reward for the first time) because the initial value will always be zero. Also, the error for that statement states "No deposits for this user", is a false statement, because it does not indicate the balance of the lender, rather, it indicates when last the lender claimed reward.

Tools Used

Manual review.

Recommended Mitigation Steps

mapping(address => mapping(address => mapping(uint256 => uint256))) public lendingMarketBalances; // cNote balances of users within the lending markets, indexed by epoch This is the variable that indicates the balance of the Lender in a particular lending market and epoch. The statement should be replaced with: require(lendingMarketBalances[_market][lender][epoch] > 0, "No deposits for this user");

Assessed type

Context