code423n4 commented 2 years ago

Handle

defsec

Vulnerability details

Impact

the _minLockPeriod parameter are used for the lock time on the deposits and withdrawals. In the state variable , proper check up should be done , other wise error in these state variable can lead to redeployment of contract. The possible huge value will definitely lock the funds.

Proof of Concept

Navigate to the following contract.

    constructor(
        IERC20 _underlying,
        uint256 _minLockPeriod,
        uint256 _investPerc,
        address _owner
    ) Trust(_owner) {
        require(
            PercentMath.validPerc(_investPerc),
            "Vault: invalid investPerc"
        );
        require(
            address(_underlying) != address(0x0),
            "VaultContext: underlying cannot be 0x0"
        );
        investPerc = _investPerc;
        underlying = _underlying;
        minLockPeriod = _minLockPeriod;

        depositors = new Depositors(address(this), "depositors", "p");
        claimers = new Claimers(address(this));
    }

minLockPeriod does not have any validation. Also, the contract does not have any setter function. Misdeployment can cause huge problems on the deposit and withdrawals.

Tools Used

Code Review

Recommended Mitigation Steps

Hardcode minLockPeriod on the contract.

r2moon commented 2 years ago

we don't need validation and it is immutable value.

dmvt commented 2 years ago

Agree with sponsor. A validation here would not help anything.

code-423n4 / 2022-01-sandclock-findings