code-423n4 / 2021-07-sherlock-findings

0 stars 0 forks source link

result of getUnstakeEntrySize is incorrect #27

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Impact

The result of the function getUnstakeEntrySize will be off if multiple calls are done to activateCooldown. Then the array unstakeEntries[msg.sender][..] will have multiple entries.

When one of the functions cancelCooldown / unstakeWindowExpiry / unstake is called, it deletes an entry from this array. However the array is never pop-ed (which also would require switching the last element with the to-be-deleted element, as is done in other parts of the code). So the array stays the same length and gets larger with every activateCooldown.

Thus the result of getUnstakeEntrySize will not be correct once activateCooldown is called at least twice.

Proof of Concept

// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/PoolBase.sol#L114 function getUnstakeEntrySize(address _staker, IERC20 _token) external view override returns (uint256) { return baseData().unstakeEntries[_staker].length; }

function activateCooldown(uint256 _amount, IERC20 _token) external override returns (uint256) { .. ps.unstakeEntries[msg.sender].push(PoolStorage.UnstakeEntry(uint40(block.number), _amount.sub(fee)) );

function cancelCooldown(uint256 _id, IERC20 _token) external override { .. delete ps.unstakeEntries[msg.sender][_id];

function unstakeWindowExpiry( address _account, uint256 _id, IERC20 _token ) external override { .. delete ps.unstakeEntries[_account][_id];

function unstake( uint256 _id, address _receiver, IERC20 _token) external override returns (uint256 amount) { ... delete ps.unstakeEntries[msg.sender][_id];

Tools Used

Recommended Mitigation Steps

Do something like the following in cancelCooldown / unstakeWindowExpiry / unstake:

ps.unstakeEntries[msg.sender][_id] = ps.unstakeEntries[msg.sender][ps.unstakeEntries[msg.sender].length - 1]; ps.unstakeEntries[msg.sender].pop();

Evert0x commented 3 years ago

The function is used to return the length of the array, not to return the amount of current unstakeEntries

ghoul-sol commented 3 years ago

per sponsor comment, invalid