stafiprotocol / security

0 stars 1 forks source link

INIT getUnstakeIndexLis #33

Closed DimaKovalenko17 closed 1 year ago

DimaKovalenko17 commented 1 year ago

Good afternoon, I found a couple of vulnerabilities in the code and bugs. This is my 3rd attempt to find bugs in the code and errors, I found some amount, maybe the code can be improved

My wallet - 0x9d06f88DF84a3A0dc04AfE8fD0a92A7f36BbBe35 1-2-3 errors, there are in the code, Below is the complete code

The getUnstakeIndexListOf function does not check values for _staker There is no event emission. The init function does not emit any events for the user No error handling. The init function does not revert if the initialization conditions are not met Non-identical variables: The latestEra and rate variables may cause unexpected behavior if they are used before they are set in contracts Unused variables: totalRTokenSupply and totalProtocolFee are declared but not used in the contract.

no input validation getValidatorldsOf does not validate the input value _poolAddress

No validation of input data. The getvBondedPools function does not check the input value bondedPools.lenght.

Lack of input data validation. The getRate function does not check input rate values

// unstake info
uint256 public nextUnstakeIndex;
mapping(uint256 => UnstakeInfo) public unstakeAtIndex;
mapping(address => EnumerableSet.UintSet) unstakesOfUser;

// events
event Stake(address staker, address poolAddress, uint256 tokenAmount, uint256 rTokenAmount);
event Unstake(
    address staker,
    address poolAddress,
    uint256 tokenAmount,
    uint256 rTokenAmount,
    uint256 burnAmount,
    uint256 unstakeIndex
);
event Withdraw(address staker, address poolAddress, uint256 tokenAmount, int256[] unstakeIndexList);
event ExecuteNewEra(uint256 indexed era, uint256 rate);
event SetUnbondingDuration(uint256 unbondingDuration);
event Delegate(address pool, uint256 validator, uint256 amount);
event Undelegate(address pool, uint256 validator, uint256 amount);
event NewReward(address pool, uint256 amount);
event NewClaimedNonce(address pool, uint256 validator, uint256 nonce);

// ** this is first vulneriblies //*** init
  1. There is no function control. Init function - no access control and checks. Potentially anyone can call it and overwrite the initial changes set by the administrator.
  2. There is no input validation for the INIT function
function init(address _rTokenAddress, address _erc20TokenAddress, uint256 

It does not check _rTokenDdress and _unbondingDuration values. _unbondingDuration) public { require(admin == address(0), "already init"); require(_rTokenAddress != address(0), "zero rtoken address"); require(_erc20TokenAddress != address(0), "zero token address"); require(_unbondingDuration <= MAX_UNBONDING_DURATION, "max unbonding duration limit");

    admin = msg.sender;
    delegationBalancer = msg.sender;
    rTokenAddress = _rTokenAddress;
    erc20TokenAddress = _erc20TokenAddress;
    unbondingDuration = _unbondingDuration;

    minStakeAmount = 1e12;
    rateChangeLimit = 5 * 1e14;
    unstakeFeeCommission = 2e15;
    protocolFeeCommission = 1e17;
    eraSeconds = 86400;
    eraOffset = 16845;
}

// modifer
modifier onlyAdmin() {
    require(admin == msg.sender, "caller is not admin");
    _;
}

modifier onlyDelegationBalancer() {
    require(delegationBalancer == msg.sender, "caller is not delegation balancer");
    _;
}

// ----- getters

function getRate() external view override returns (uint256) {
    return rate;
}

function getBondedPools() public view returns (address[] memory pools) {
    pools = new address[](bondedPools.length());
    for (uint256 i = 0; i < bondedPools.length(); ++i) {
        pools[i] = bondedPools.at(i);
    }
    return pools;
}

function getValidatorIdsOf(address _poolAddress) public view returns (uint256[] memory validatorIds) {
    validatorIds = new uint256[](validatorIdsOf[_poolAddress].length());
    for (uint256 i = 0; i < validatorIdsOf[_poolAddress].length(); ++i) {
        validatorIds[i] = validatorIdsOf[_poolAddress].at(i);
    }
    return validatorIds;
}

function getUnstakeIndexListOf(address _staker) external view returns (uint256[] memory unstakeIndexList) {
    unstakeIndexList = new uint256[](unstakesOfUser[_staker].length());
    for (uint256 i = 0; i < unstakesOfUser[_staker].length(); ++i) {
        unstakeIndexList[i] = unstakesOfUser[_staker].at(i);
    }
    return unstakeIndexList;
}
Tore19 commented 1 year ago
  1. The getUnstakeIndexListOf function does not check values for _staker
  2. no input validation getValidatorldsOf does not validate the input value _poolAddress
  3. No validation of input data. The getvBondedPools function does not check the input value bondedPools.lenght.
  4. Lack of input data validation. The getRate function does not check input rate values

There is no need to check the input parameters of these get methods, and it has no effect on the function.

  1. There is no event emission. The init function does not emit any events for the user

The init function will only be called once, the event is not necessary and can be omitted.

  1. No error handling. The init function does not revert if the initialization conditions are not met

The require function can check the initialization conditions. So no need to use revert.

  1. Non-identical variables: The latestEra and rate variables may cause unexpected behavior if they are used before they are set in contracts

Before the contract function is used, the migrate function will be called, where the latestEra and rate variables will be set. So we believe there will be no problems.

  1. Unused variables: totalRTokenSupply and totalProtocolFee are declared but not used in the contract.

These can help to check the accurate rToken supply and protocol fee conveniently, without any impact on the contract function.