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

0 stars 0 forks source link

Avoid unnecessary `updateStream()` can save gas #256

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

updateStream() includes complex code execution, avoiding unnecessary internal call of updateStream() can save gas.

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

function exit() public updateStream(msg.sender) {
    // checked in updateStream
    // is the stream still going on? thats the only time a depositer can withdraw
    // require(block.timestamp < endStream, "withdraw:!stream");
    TokenStream storage ts = tokensNotYetStreamed[msg.sender];
    uint112 amount = ts.tokens;
    withdraw(amount);
}

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

function withdraw(uint112 amount) public lock updateStream(msg.sender) {
    require(amount > 0, "amt");

    // checked in updateStream
    // is the stream still going on? thats the only time a depositer can withdraw
    // require(block.timestamp < endStream, "withdraw:!stream");
    TokenStream storage ts = tokensNotYetStreamed[msg.sender];

    require(ts.tokens >= amount, "amt");
    ts.tokens -= amount;

    uint256 virtualBal = dilutedBalance(amount);
    ts.virtualBalance -= virtualBal;
    totalVirtualBalance -= virtualBal;
    depositTokenAmount -= amount;
    if (!isSale) {
        _burn(msg.sender, amount);
    } else {
    }

    // do the transfer
    ERC20(depositToken).safeTransfer(msg.sender, amount);

    emit Withdrawn(msg.sender, amount);
}

updateStream(msg.sender) at L487 is duplicated with updateStream(msg.sender) in withdraw() at L493.

Recommendation

Change to:

    function withdraw(uint112 amount) public lock updateStream(msg.sender) {
        _withdraw(amount);
    }

    function _withdraw(uint112 amount) private {
        require(amount > 0, "amt");

        // checked in updateStream
        // is the stream still going on? thats the only time a depositer can withdraw
        // require(block.timestamp < endStream, "withdraw:!stream");
        TokenStream storage ts = tokensNotYetStreamed[msg.sender];

        require(ts.tokens >= amount, "amt");
        ts.tokens -= amount;

        uint256 virtualBal = dilutedBalance(amount);
        ts.virtualBalance -= virtualBal;
        totalVirtualBalance -= virtualBal;
        depositTokenAmount -= amount;
        if (!isSale) {
            _burn(msg.sender, amount);
        } else {
        }

        // do the transfer
        ERC20(depositToken).safeTransfer(msg.sender, amount);

        emit Withdrawn(msg.sender, amount);
    }

    /**
     *  @dev Allows a stream depositor to exit their entire remaining tokens that haven't streamed
     *  and burns receiptTokens if its not a sale.
     * 
     *  additionally, updates tokensNotYetStreamed. Lock is done in withdraw
    */ 
    function exit() public lock updateStream(msg.sender) {
        // checked in updateStream
        // is the stream still going on? thats the only time a depositer can withdraw
        // require(block.timestamp < endStream, "withdraw:!stream");
        TokenStream storage ts = tokensNotYetStreamed[msg.sender];
        uint112 amount = ts.tokens;
        _withdraw(amount);
    }
0xean commented 2 years ago

dupe of #187