ravikiran.web3 - PointTokenVault::execute() function does not check the status of the delegate call and revert on failure

[sherlock-admin3]

ravikiran.web3

Medium

PointTokenVault::execute() function does not check the status of the delegate call and revert on failure

Summary

PointTokenVault::execute(...) makes a delegate call and the status of the delegate call is returned to the caller as boolean. It is essentially transferring the responsibility to check the status of the delegate call to the caller itself. This is risk if the caller does not perform the necessary check and executes other logic assuming PointTokenVault::execute(...) ran fine.

It is recommended to revert incase the delegate call was not successful blocking the whole transaction.

Root Cause

In the below code snippet, the status for delegate call is read and passed back to the caller.
The recommendation is to revert in the PointTokenVault::execute(...) if the delegate function fails.

https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/main/point-tokenization-vault/contracts/PointTokenVault.sol#L365-L373

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

As PointTokenVault::execute(...) is an external function and hence might not be able to enforce a revert return, it is recommended that the PointTokenVault::execute(...) reverts incase the delegate call fails.

PoC

No response

Mitigation

Revise the execute function as below.

 function execute(address _to, bytes memory _data, uint256 _txGas)
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        returns (bool success)
    {
        assembly {
            success := delegatecall(_txGas, _to, add(_data, 0x20), mload(_data), 0, 0)
        }
        require(success,"Delegate Call Failed");
    }

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
sense-finance/point-tokenization-vault#42

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.