veRWA - KmanOfficial's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L288-L323 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L356-L387 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L268-L284

Vulnerability details

Impact

Creating a lock (createLock) allows the user to lock CANTO tokens for a fixed time of 5 years from the current block.timestamp of when the transaction is executed on chain and this is rounded down to the nearest week relative to block.timestamp + 1825 days. The user can choose to delegate their locked CANTO and hence voting power to another address if the created lock of the delegate address ends at the same time or after the user's own created lock.

If the user only locks and delegates and doesn't choose to increaseAmount of CANTO that they lock during their initial locking period, when both the msg.sender (user) and delegatee's lock end timestamp has been reached the end user will not be able to remove undelegate and be able to withdraw.

The user also wouldn't be able to create a new lock either or increase the amount within their current lock in order to extend the lock time by another 5 years to allow the undelegate action. This is because in the first scenario as there is already tokens locked, createLock would revert (require(locked_.amount == 0, "Lock exists");). In the second scenario, you can only increase the amount within the lock if the lock has not expired which in this scenario it would of expired (require(locked_.end > block.timestamp, "Lock expired");).

This means user funds could be stuck in the VotingEscrow contract.

Proof of Concept

Pseudo Scenario:

• Entity A locks 10 CANTO which creates a lock that expires in 5 years. 2 weeks passes
• Entity B locks 100 CANTO, which creates a lock that expires in 5 years. (Expires 5 years + 2 weeks later relative to Entity A)
• Entity A decides to delegate their locked CANTO to Entity B. This is allowed as Entity B's lock end is greater than Entity A. 5 years + 2 weeks passes
• Entity A wants to undelegate their locked CANTO and withdraw - unable to delegate because of the condition in the delegate() function leading to a revert (https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L383)

With Entity A unable to create a new lock or increase amount within lock, albeit these should be unnecessary actions to recover their funds, their CANTO is permanently locked within in the contract. They can only delegate to another address that has a current lock, but will still nevertheless be able to withdraw.

Tools Used

Manual Code Review, VS Code

Recommended Mitigation Steps

Allow un-delegation either unconditionally or if both the delegatee and the user's lock have both expired depending on how the protocol wants to handle this.

Assessed type

Other

[QA-1] "//See IVotingEscrow for documentation" comment should be removed and description of each function should be added as there isn't an interface for VotingEscrow in this project or IVotingEscrow.sol should be added and utilised.

As this was forked or inspired by the FIAT DAO repo, this comment was most likely from there. It should be removed and replaced with the function descriptions for more clarity when reading through the contract.

Instances:

https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L267 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L286 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L327 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L360 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L477 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L491 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L569 https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L576

[QA-2] createLock has an unnecessary nonReentrant Modifier

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

This function has no route or possibility for an arbitrary call to take place that would allow for an reentrancy attack so it is unnecessary despite it not affecting execution flow. It could be removed to save on unnecessary gas execution and for code principle.