code-423n4 / 2021-11-streaming-findings

0 stars 0 forks source link

Avoid unnecessary external calls can save gas #262

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L696-L724

    function flashloan(address token, address to, uint112 amount, bytes memory data) public lock {
        require(token == depositToken || token == rewardToken, "erc");

        uint256 preDepositTokenBalance = ERC20(depositToken).balanceOf(address(this));
        uint256 preRewardTokenBalance = ERC20(rewardToken).balanceOf(address(this));

        ERC20(token).safeTransfer(to, amount);

        // the `to` contract should have a public function with the signature:
        // function lockeCall(address initiator, address token, uint256 amount, bytes memory data);
        LockeCallee(to).lockeCall(msg.sender, token, amount, data);

        uint256 postDepositTokenBalance = ERC20(depositToken).balanceOf(address(this));
        uint256 postRewardTokenBalance = ERC20(rewardToken).balanceOf(address(this));

        uint112 feeAmt = amount * 10 / 10000; // 10bps fee

        if (token == depositToken) {
            depositTokenFlashloanFeeAmount += feeAmt;
            require(preDepositTokenBalance + feeAmt <= postDepositTokenBalance, "f1");
            require(preRewardTokenBalance <= postRewardTokenBalance, "f2");
        } else {
            rewardTokenFeeAmount += feeAmt;
            require(preDepositTokenBalance <= postDepositTokenBalance, "f3");
            require(preRewardTokenBalance + feeAmt <= postRewardTokenBalance, "f4");
        }

        emit Flashloaned(token, msg.sender, amount, feeAmt);
    }

Given that only one token is related, reading the balanceOf of another token is unnecessary.

Recommendation

Change to:

    function flashloan(address token, address to, uint112 amount, bytes memory data) public lock {
        require(token == depositToken || token == rewardToken, "erc");

        uint256 preTokenBalance = ERC20(token).balanceOf(address(this));

        ERC20(token).safeTransfer(to, amount);

        // the `to` contract should have a public function with the signature:
        // function lockeCall(address initiator, address token, uint256 amount, bytes memory data);
        LockeCallee(to).lockeCall(msg.sender, token, amount, data);

        uint256 postTokenBalance = ERC20(token).balanceOf(address(this));

        uint112 feeAmt = amount * 10 / 10000; // 10bps fee

        if (token == depositToken) {
            depositTokenFlashloanFeeAmount += feeAmt;
        } else {
            rewardTokenFeeAmount += feeAmt;
        }
        require(preTokenBalance + feeAmt <= postTokenBalance, "fee");

        emit Flashloaned(token, msg.sender, amount, feeAmt);
    }
0xean commented 2 years ago

I believe this to be a security measure, but will leave as valid so the sponsor can decide.