Description

Whenever the owner of the StakingPool contract calls withdraw(), the indexStar variable is set to be equal to indexEnd.

function withdraw() public onlyOwner {
    // ...
    indexStar = indexEnd;
    // ...
}

These two variables represent the cut-off index for withdrawn stakes and the next index to assign when someone stakes; indexStar is initialized to 0 and indexEnd starts at 1. During a stake, the indexEnd value is used, then increased. The first stake will be put into the issues mapping at index 1, and indexEnd++ will be 2.

function stake(uint256 amount) public NOT_AMERICAN {
    // ...
    issues[indexEnd] = Issue(msg.sender, amount, block.timestamp, true);
    userIssueIndex[msg.sender].push(indexEnd);
    indexEnd++;
    // ...
    }

During unstake, there is a check that the index fetched from userIssueIndex is larger than indexStar.

function unstake() public NOT_AMERICAN {
    uint[] memory userIssueIndexs = userIssueIndex[msg.sender];

    uint unstakeAmount;
    for (uint i; i < userIssueIndexs.length; i++) {
        uint index = userIssueIndexs[i];
        if (index > indexStar) {
            // Unstake
        }
    }
    // ...
}

The point of this is that all issues should be invalidated when a withdraw happens, but because indexEnd points to the next issue index, it will also invalidate the next stake, which has not happened yet. A similar check happens in swap, where it will count down from the end of the issues, starting at an invalid index and counting down to the value above indexStar.

for (uint i = indexEnd; i > indexStar; i--) {
    Issue storage issueInfo = issues[i];
    if (issueInfo.isStaking) {
        // ...
    }
    // ...
}

Impact

The first stake that happens after a withdraw cannot be unstaked and will not be considered for swap. These funds are essentially lost unless the owner calls withdraw again and manually gives the stake back to the user. But doing this will trigger the bug again for the next stake.

The swap will also read one invalid issue each time, as issues[indexEnd] is never actually assigned, except for a brief moment in stake until indexEnd++ is called right after.

Recommendations

Rename indexEnd to something like nextIndex to reflect what it actually is, and make sure it is always strictly larger than indexStar. In the withdraw function, set indexStar to nextIndex - 1 or similar. Change any loops to not start at this exact index, as it is not yet valid.

Write test cases for more complicated scenarios that involve multiple users, doing multiple stakes and unstakes, both before and after the admin calls withdraw.

Remediation

This issue has been acknowledged by Chateau Capital, and fixes were implemented in the following commits:

• c296913a↗
• 8a221f45↗