29015 - [SC - Low] Boolean return value of addresscall function no...

Submitted on Mar 4th 2024 at 19:36:27 UTC by @kaysoft for Boost | Puffer Finance

Report ID: #29015

Report type: Smart Contract

Report severity: Low

Target: https://etherscan.io/address/0x3C28B7c7Ba1A1f55c9Ce66b263B33B204f2126eA#code

Impacts:

• Contract fails to deliver promised returns, but doesn't lose value

Description

Brief/Intro

The Timelock.sol contract's executeTransfaction(...) function implements a target.call() function without checking the return value for the success of the transaction.

When an address.call is made to a function of a contract and it reverts, the error is not bubbled up but instead returns a false return value. Not checking the boolean return value causes the sender of the transaction to assume the call is successful until the user checks manually because the whole transaction will be successful.

Vulnerability Details

When exceptions happen in an address.call(), they do not "bubble up" but instead return a false boolean value.

That should be checked with a require() statement.

The executeTransaction() external function of Timelock.sol calls the internal _executeTransaction. The issue lies in the fact that _executeTransaction makes a target.call() call without checking the returned boolean value. The internal _executeTransaction just returns the values returned from the target.call(). Also the executeTransaction() external function did not check the returned boolean value either making the whole transaction successful.

This will make the user assume that target.call() was also successfully executed unless manually checked.

File: TimeLock.sol
function _executeTransaction(address target, bytes calldata callData) internal returns (bool, bytes memory) {
        // slither-disable-next-line arbitrary-send-eth
        return target.call(callData);
    }

File: Timelock.sol
function executeTransaction(address target, bytes calldata callData, uint256 operationId)
        external
        returns (bool success, bytes memory returnData)
    {
        // Community Multisig can do things without any delay
        if (msg.sender == COMMUNITY_MULTISIG) {
            return _executeTransaction(target, callData); //@audit comm multisig can execute non queued transaction.
        }

        // Operations multisig needs to queue it and then execute after a delay
        if (msg.sender != OPERATIONS_MULTISIG) {
            revert Unauthorized();
        }

        bytes32 txHash = keccak256(abi.encode(target, callData, operationId));
        uint256 lockedUntil = queue[txHash];

        // slither-disable-next-line incorrect-equality
        if (lockedUntil == 0) {
            revert InvalidTransaction(txHash);
        }

        if (block.timestamp < lockedUntil) {
            revert Locked(txHash, lockedUntil);
        }

        queue[txHash] = 0;
        (success, returnData) = _executeTransaction(target, callData);

        emit TransactionExecuted(txHash, target, callData, operationId);

        return (success, returnData);//@audit success not checked
    }

Impact Details

Failure of target.call() not validated which will make the user assume success of the target.call() unless the transaction and the contract is manually checked.

File: TimeLock.sol
function _executeTransaction(address target, bytes calldata callData) internal returns (bool, bytes memory) {
        // slither-disable-next-line arbitrary-send-eth
--        return target.call(callData);
++        (boolean success, bytes memory data) = target.call(callData);
++        require(success, "Timelock: Call failed");
++        return (success, data);
    }

##Recommendation Consider adding a check for target.call() and also add a contract existence check for target contract.

References

• https://github.com/PufferFinance/pufETH/blob/2768d69196717e9f77a6837153b426e06e15c51f/src/Timelock.sol#L265

• https://docs.soliditylang.org/en/latest/control-structures.html#error-handling-assert-require-revert-and-exceptions

Proof of Concept

This POC can be added to the Timelock.t.sol file

This transaction sets the newPauser as zero address which should normally revert due to the require statement in the setPauser function but it does not revert because the boolean return value is not checked. This creates a successful transaction which can make the user think the new pauser is set.

function test_change_pauser() public {

        vm.startPrank(timelock.COMMUNITY_MULTISIG());

        bytes memory callData = abi.encodeCall(Timelock.setPauser, address(0));

       //This transaction does not revert and its successful
        timelock.executeTransaction(address(timelock), callData, 1234);

    }