Skip to content

Commit

Permalink
fix: check for locker and delegate addresses (#392)
Browse files Browse the repository at this point in the history
### Description

fixes[[M-1] Mento tokens can be lost, or delegated incorrectly in
`lock()`
function](https://www.notion.so/0xmacro/Mento-3-Preliminary-Audit-Report-external-private-06ee6ce11d7d4df0a950ca97c75a2766?pvs=4#21daa0832c364978a2227fb05fba34f2)

- added zero address check for locker and delegate in lock() as
suggested
- also added checks for delegates in delegateTo() and relock() because
the same issue is present in those functions

### Other changes
No

### Tested

- New unit tests implemented
- Old tests pass

### Related issues

- Fixes [#377](#377)
  • Loading branch information
baroooo authored Mar 6, 2024
1 parent 220b5b8 commit 085a021
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 1 deletion.
6 changes: 5 additions & 1 deletion contracts/governance/locking/Locking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ contract Locking is ILocking, LockingBase, LockingRelock, LockingVotes {
/**
* @notice Locks a specified amount of tokens for a given period
* @dev Can not be called when locking is stopped
* @dev Delegate is not optional, it can be set to the lock owner if no delegate is desired
* @param account Account for which tokens are being locked
* @param _delegate Address that will receive the voting power from the locked tokens
* If address(0) passed, voting power will be lost
* @param amount Amount of tokens to lock
* @param slopePeriod Period over which the tokens will unlock
* @param cliff Initial period during which tokens remain locked and do not start unlocking
Expand All @@ -73,6 +73,8 @@ contract Locking is ILocking, LockingBase, LockingRelock, LockingVotes {
require(amount > 0, "zero amount");
require(cliff <= MAX_CLIFF_PERIOD, "cliff too big");
require(slopePeriod <= MAX_SLOPE_PERIOD, "period too big");
require(account != address(0), "account is zero");
require(_delegate != address(0), "delegate is zero");

counter++;

Expand Down Expand Up @@ -153,6 +155,8 @@ contract Locking is ILocking, LockingBase, LockingRelock, LockingVotes {
* @param newDelegate The address to which the delegation will be transferred
*/
function delegateTo(uint256 id, address newDelegate) external notStopped {
require(newDelegate != address(0), "delegate is zero");

address account = verifyLockOwner(id);
address _delegate = locks[id].delegate;
uint32 currentBlock = getBlockNumber();
Expand Down
2 changes: 2 additions & 0 deletions contracts/governance/locking/LockingRelock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ abstract contract LockingRelock is LockingBase {
uint32 newSlopePeriod,
uint32 newCliff
) external notStopped returns (uint256) {
require(newDelegate != address(0), "delegate is zero");

address account = verifyLockOwner(id);
uint32 currentBlock = getBlockNumber();
uint32 time = roundTimestamp(currentBlock);
Expand Down
13 changes: 13 additions & 0 deletions test/governance/Locking/delegateTo.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,19 @@ import { Locking_Test } from "./Base.t.sol";
contract DelegateTo_Locking_Test is Locking_Test {
uint256 public lockId;

function test_delegateTo_whenDelegateZero_shouldRevert() public {
mentoToken.mint(alice, 100000);

vm.prank(alice);
lockId = locking.lock(alice, bob, 60000, 30, 0);

_incrementBlock(20 * weekInBlocks);

vm.prank(alice);
vm.expectRevert("delegate is zero");
locking.delegateTo(lockId, address(0));
}

function test_delegateTo_whenReDelegateToDifferentAccount_shouldDelegateCorrectly() public {
mentoToken.mint(alice, 100000);

Expand Down
16 changes: 16 additions & 0 deletions test/governance/Locking/locking.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,22 @@ contract Lock_Locking_Test is Locking_Test {
locking.lock(alice, alice, 20, 40, 0);
}

function test_lock_whenAccountZero_shouldRevert() public {
mentoToken.mint(alice, 1500);

vm.expectRevert("account is zero");
vm.prank(alice);
locking.lock(address(0), alice, 20, 40, 0);
}

function test_lock_whenDelegateZero_shouldRevert() public {
mentoToken.mint(alice, 1500);

vm.expectRevert("delegate is zero");
vm.prank(alice);
locking.lock(alice, address(0), 20, 40, 0);
}

function test_withdraw_whenInSlope_shouldReleaseCorrectAmount() public {
mentoToken.mint(alice, 1000);

Expand Down
13 changes: 13 additions & 0 deletions test/governance/Locking/relock.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,19 @@ import { Locking_Test } from "./Base.t.sol";
contract Relock_Locking_Test is Locking_Test {
uint256 public lockId;

function test_relock_whenDelegateZero_shouldRevert() public {
mentoToken.mint(alice, 100);

vm.prank(alice);
lockId = locking.lock(alice, alice, 30, 3, 3);

_incrementBlock(2 * weekInBlocks);

vm.expectRevert("delegate is zero");
vm.prank(alice);
locking.relock(lockId, address(0), 30, 3, 3);
}

function test_relock_whenInCliff_shouldRelockWithIncreasedAmount() public {
mentoToken.mint(alice, 100);

Expand Down

0 comments on commit 085a021

Please sign in to comment.