VTVL contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L176

Vulnerability details

Impact

when calculate the vested amount, if linearVestAmount and endTimestamp are large will lead to overflow uint112, the token that has entered the contract will be locked in the contract

Proof of Concept

Assume that after 1 year to claim,token decimals is 18 Then as long as the user saves: type(uint112).max / 365 days / 10**18 = 164646653 if more than 164646653 tokens can create "Claim" successfully , but withdraw will overflow and cause the token to be locked this number level is still very possible, if the time is longer , that linearVestAmount can be smaller

    function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) {
        ...

        //**** overflow (uint112*uint40) *****//
        // linearVestAmount > 164646653* 10**18 and truncatedCurrentVestingDurationSecs = 365 days
        uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;
       ....

The overflow causes both withdraw() and revokeClaim() to fail, and the token is permanently locked in the contract

Recommended Mitigation Steps

Convert to uint256,calculate and then convert back to uint112

    function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) {
        ...

---    uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;
+++    uint256 linearVestAmount = uint256(_claim.linearVestAmount) * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;

        // Having calculated the linearVestAmount, simply add it to the vested amount
---     vestAmt += linearVestAmount;        
+++     vestAmt += uint112(linearVestAmount);
+++     //cast to uint112 is safe, linearVestAmount will not be greater than type(uint112).max , because finalVestingDurationSecs >= truncatedCurrentVestingDurationSecs

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L43

Vulnerability details

Impact

VariableSupplyERC20Token uses mintableSupply to control the maximum mint, but the error of each mint will reduce this value accordingly, and when mintableSupply reduct to 0, there is no limit again, you can mint at will

Proof of Concept

1.init mintableSupply = 100 2.mint 100 token 3.after mint, mintableSupply = 0 , you can mint at will

    function mint(address account, uint256 amount) public onlyAdmin {
        require(account != address(0), "INVALID_ADDRESS");
        // If we're using maxSupply, we need to make sure we respect it
        // mintableSupply = 0 means mint at will
        if(mintableSupply > 0) {
            require(amount <= mintableSupply, "INVALID_AMOUNT");
            // We need to reduce the amount only if we're using the limit, if not just leave it be
            mintableSupply -= amount; //******** reduce，when mintableSupply=0，you can mint at will********//
        }
        _mint(account, amount);
    }

Recommended Mitigation Steps

don't reduce mintableSupply , check with totalSupply

    function mint(address account, uint256 amount) public onlyAdmin {
        require(account != address(0), "INVALID_ADDRESS");
        // If we're using maxSupply, we need to make sure we respect it
        // mintableSupply = 0 means mint at will
---     if(mintableSupply > 0) {
---         require(amount <= mintableSupply, "INVALID_AMOUNT");
---         // We need to reduce the amount only if we're using the limit, if not just leave it be
---         mintableSupply -= amount; 
---     }
        _mint(account, amount);
+++     if(mintableSupply > 0) {
+++         require(totalSupply() <= mintableSupply, "INVALID_AMOUNT");
+++     }                
    }

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L39

Vulnerability details

Impact

admin can cancel themselves, which may result in no admin and losing control of the contract forever

Proof of Concept

setAdmin() is used to add/reduce/replace administrators The normal when replacing is :

1. set someone's to true
2. set your own to false If you accidentally set your own to false first, it will lead to no administrator, so it is recommended to cancel someone's administrator privileges can only be operated by others, you can not cancel yourself, so as to ensure that there is at least one administrator

Recommended Mitigation Steps

add check

    function setAdmin(address admin, bool isEnabled) public onlyAdmin {
           require(admin != address(0), "INVALID_ADDRESS");
+++     require((admin!=_msgsender(),"NOT SELF");
+++     require(_admins[admin]!=isEnable,"NOT CHANGE");
        
        _admins[admin] = isEnabled;
        emit AdminAccessSet(admin, isEnabled);
    }