stkBNB - Persistence

The _claim function checks the amount to withdraw from the protocol is less than the current contract balance. However, this check is not correct, and it can lead to revert the transaction as the claimReserve can be 0, although the contract balance is not 0. So on the subtraction on L786, the contract reverts with an unhandled arithmetic exception.

Code Location

StakePool.sol

if (address(this).balance < req.weiToReturn) {
    revert InsufficientFundsToSatisfyClaim();
}

SOLVED: The persistenceOne team fixed the above issue on the commit ID d059bccbb368158a63767107b37894d47009c385.

The StakePool contracts performs unsafe casting from uint256 to int256 that can lead to overflow. Although the risk is minimum as the amount of BNB is still far from reaching those numbers.

Code Location

StakePool.sol

if (_bnbToUnbond > int256(excessBNB)) {

StakePool.sol

_bnbToUnbond -= int256(shortCircuitAmount);

StakePool.sol

_bnbToUnbond -= int256(bnbUnbonding_);

StakePool.sol

_bnbToUnbond += int256(weiToReturn);

SOLVED: The persistenceOne team fixed the above issue on the commit ID d059bccbb368158a63767107b37894d47009c385. The final implementation contains the recommended library to avoid overflows on arithmetic operations when casting.

The claimStkBNB function from the FeeVault contract can send the fees to the StakePool contract. Although this action is restricted by the onlyOwner modifier, this action may lead the fees to be locked on the StakePool contract forever. This happens because there is no way to call the claim function from the FeeVault contract.

Code Location

FeeVault.sol

function claimStkBNB(address recipient, uint256 amount) external override onlyOwner {
        IStakedBNBToken(addressStore.getStkBNB()).send(recipient, amount, "");

        emit Withdraw(msg.sender, recipient, amount);
    }

SOLVED: The persistenceOne team fixed the above issue on the commit ID d059bccbb368158a63767107b37894d47009c385.

The usage of send or transfer limits the amount of gas send in the transaction to 2300. This can be considered a safeguard to avoid reentrancy. However, given the current protections of the contracts, it does seem feasible to use call function without further risk. This implies that the system expects to be used by some contracts that can execute more complex code on the fallback function.

Code Location

StakePool.sol

    function _claim(uint256 index) internal returns (bool) {
        if (index >= claimReqs[msg.sender].length) {
            revert IndexOutOfBounds(index);
        }

        // find the requested claim
        ClaimRequest memory req = claimReqs[msg.sender][index];

        if (!_canBeClaimed(req)) {
            return false;
        }
        if (_claimReserve < req.weiToReturn) {
            revert InsufficientFundsToSatisfyClaim();
        }

        // update _claimReserve
        _claimReserve -= req.weiToReturn;

        // delete the req, as it has been fulfilled (swap deletion for O(1) compute)
        claimReqs[msg.sender][index] = claimReqs[msg.sender][claimReqs[msg.sender].length - 1];
        claimReqs[msg.sender].pop();

        // return BNB back to user
        payable(msg.sender).transfer(req.weiToReturn);
        emit Claim(msg.sender, req, block.timestamp);
        return true;
    }

SOLVED: The persistenceOne team fixed the above issue on the commit ID d059bccbb368158a63767107b37894d47009c385.

The AddressStore contract use Ownable from OpenZeppelin which is a simple mechanism to transfer the ownership not supporting a two-step transfer ownership pattern. Ownable is a simpler mechanism with a single owner "role" that can be assigned to a single account. This simpler mechanism can be useful for quick tests, but projects with production concerns are likely to outgrow it. Transferring ownership is a critical operation and this could lead to transferring it to an inaccessible wallet or renouncing the ownership, e.g. mistakenly.

Code Location

AddressStore Contract

AddressStore.sol

contract AddressStore is IAddressStore, Ownable

SOLVED: The persistenceOne team does not require a fix for this, as the platform will use a multi-signature wallet when developed on production.

Calling a contract, although a view function, still requires some gas from the contract. The UndelegationHolder contract performs the call to the StakePool contract twice instead of storing the result on a variable.

Code Location

UndelegationHolder contract

UndelegationHolder.sol

if (amountToSend > IStakePoolBot(stakePool).bnbUnbonding()) {
    amountToSend = IStakePoolBot(stakePool).bnbUnbonding();
}

SOLVED: The persistenceOne team fixed the above issue on the commit ID d059bccbb368158a63767107b37894d47009c385.

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

Code Location

StakePool contract

StakePool.sol

_TOKEN_HUB.transferOut{ value: excessBNB }(
    _ZERO_ADDR,
    config.bcStakingWallet,
    transferOutAmount,
    uint64(block.timestamp + 3600)
);

ACKNOWLEDGED: The persistenceOne team acknowledged this issue.

The function getPaginatedClaimRequests uses i++ which costs more gas than ++i, especially in a loop. In the loops below, postfix (e.g. i++) operators were used to increment or decrement variable values. It is known that, in loops, using prefix operators (e.g. ++i) costs less gas per iteration than using postfix operators.

Code Location

StakePool Contract

StakePool.sol

for (uint256 i = 0; i < to - from; i++) {
            paginatedClaimRequests[i] = claimReqs[user][from + i];
}

SOLVED: The persistenceOne team fixed the above issue on the commit ID d059bccbb368158a63767107b37894d47009c385.