Closed sherlock-admin4 closed 5 months ago
0xumarkhatab
medium
msg.sender
Whenever PerennialAction.EXEC_ORDER is to be performed , _executeOrder is called with decoded params
The Natspec of the function _executeOrder states that
/// @notice executes an 'account's' open order for a `market` and pays a fee to 'msg.sender'
However nothing is paid to msg.sender
Inside Invoke method , the contract calls internal method _invoke
function _invoke(address account, Invocation[] calldata invocations) private { //..snip } else if (invocation.action == PerennialAction.EXEC_ORDER) { (address execAccount, IMarket market, uint256 nonce) = abi.decode(invocation.args, (address, IMarket, uint256)); _executeOrder(execAccount, market, nonce); } //..snip }
which for EXEC_ORDER action calls _executeOrder method.
EXEC_ORDER
_executeOrder
The function is intended to perform following task
executes an `account's` open order for a `market` and pays a fee to `msg.sender`
the tasks are
While the first might be implemented correctly , the function does not pay fee to msg.sender .
Let's dive into the function calls chain.
if we look at the code of _executeOrder
https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L368-L408
The code that is responsible for paying the fee to msg.sender is following function call
https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L373-L384
Which , if we take a look at the values provided , we see the function call as follows
Here is the signature of called function
https://github.com/equilibria-xyz/root/blob/f4b832b22da8c9969e7dc5d8e9c4163f51f45ce8/contracts/attribute/Kept/Kept.sol#L99-L105
Plus the struct definition of the keeper config is as follows
struct KeepConfig { UFixed18 multiplierBase; uint256 bufferBase; UFixed18 multiplierCalldata; uint256 bufferCalldata; }
So it means the function call resolves to this
_handleKeeperFee( config : { multiplierBase:0; bufferBase:keepBufferBase; multiplierCalldata:0; bufferCalldata:some_calldata; } , applicableGas =0, memory applicableCalldata =0 length byte due , applicableValue = 0, data = some_data )
Now when we pass these params to the target function , we go one level deep to calculate the output of this function
https://github.com/equilibria-xyz/root/blob/f4b832b22da8c9969e7dc5d8e9c4163f51f45ce8/contracts/attribute/Kept/Kept.sol#L99-L116
Let's take a look at calculating baseFee and calldata fee
(UFixed18 baseFee, UFixed18 calldataFee) = ( _baseFee(applicableGas, config.multiplierBase, config.bufferBase), _calldataFee(applicableCalldata, config.multiplierCalldata, config.bufferCalldata) );
function _baseFee( uint256 applicableGas, UFixed18 multiplierBase, uint256 bufferBase ) internal view returns (UFixed18) { return _fee(applicableGas, multiplierBase, bufferBase, block.basefee); } function _fee(uint256 gas, UFixed18 multiplier, uint256 buffer, uint256 baseFee) internal pure returns (UFixed18) { return UFixed18Lib.from(gas).mul(multiplier).add(UFixed18Lib.from(buffer)).mul(UFixed18.wrap(baseFee)); }
return UFixed18Lib.from(gas).mul(multiplier).add(UFixed18Lib.from(buffer)).mul(UFixed18.wrap(baseFee));
_baseFee(0,0,some_buffer_base) -
this results in
(0x0 + keeper_buffer_base)block.baseFee
If we take a look at constructor
constructor( Token6 usdc_, Token18 dsu_, IFactory marketFactory_, IFactory vaultFactory_, IBatcher batcher_, IEmptySetReserve reserve_, uint256 keepBufferBase_, uint256 keepBufferCalldata_ ) { USDC = usdc_; DSU = dsu_; marketFactory = marketFactory_; vaultFactory = vaultFactory_; batcher = batcher_; reserve = reserve_; keepBufferBase = keepBufferBase_; keepBufferCalldata = keepBufferCalldata_; }
there is not enforcement on non-zero value of keepBufferBase , which means It can be zero.
keepBufferBase
so out baseFee returned will be
(0x0 + 0)xBlock.baseFee = 0
So baseFee = 0
For calldataFee, it is rather simple , it always return zero
calldataFee
function _calldataFee( bytes memory applicableCalldata, UFixed18 multiplierCalldata, uint256 bufferCalldata ) internal view virtual returns (UFixed18) { return UFixed18Lib.ZERO; }
coming back to our handleKeeperFee function
we have baseFee = 0 , calldataFee=0
Now following lines remain
UFixed18 keeperFee = UFixed18.wrap(applicableValue).add(baseFee).add(calldataFee).mul(_etherPrice()); keeperFee = _raiseKeeperFee(keeperFee, data); keeperToken().push(msg.sender, keeperFee); emit KeeperCall(msg.sender, applicableGas, applicableValue, baseFee, calldataFee, keeperFee);
keeperFee will be (0x0+0)xsomeEtherPrice => 0 because applicableValue=0 , baseFee=0,calldataFee=0
(0x0+0)xsomeEtherPrice
0
So now keeperFee=0
Now _raiseKeeperFee is called with zero keeperFee
_raiseKeeperFee
keeperFee
function _raiseKeeperFee(UFixed18 keeperFee, bytes memory data) internal virtual override returns (UFixed18) { (address account, IMarket market, UFixed6 fee) = abi.decode(data, (address, IMarket, UFixed6)); UFixed6 raisedKeeperFee = UFixed6Lib.from(keeperFee, true).min(fee); _marketWithdraw(market, account, raisedKeeperFee); return UFixed18Lib.from(raisedKeeperFee); }
which will return a minimum of 0 and some non zero value, and we know the returned value will be zero
UFixed6 raisedKeeperFee = UFixed6Lib.from(keeperFee, true).min(fee);
back to rest of code with keeperFee=0
keeperToken().push(msg.sender, keeperFee); emit KeeperCall(msg.sender, applicableGas, applicableValue, baseFee, calldataFee, keeperFee);
zero fee will be paid to msg.sender and the event will be emitted
msg.sender is not paid the fee when the function is developed for that purpose : Being inconsistent with the Natspec and
will cause future integration issues in future.
https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L368-L384
Manual Review
Ensure keepBufferBase is non-zero in constructor.
1 comment(s) were left on this issue during the judging contest.
z3s commented:
Invalid; Fee handled in _handleKeeperFee;
0xumarkhatab
medium
PerennialAction.EXEC_ORDER _executeOrder does not pay fee to
msg.sender
conflicting Natspec when keepBufferBase=0Summary
Whenever PerennialAction.EXEC_ORDER is to be performed , _executeOrder is called with decoded params
The Natspec of the function _executeOrder states that
However nothing is paid to
msg.sender
Vulnerability Detail
Inside Invoke method , the contract calls internal method _invoke
which for
EXEC_ORDER
action calls_executeOrder
method.The function is intended to perform following task
the tasks are
While the first might be implemented correctly , the function does not pay fee to msg.sender .
Let's dive into the function calls chain.
if we look at the code of
_executeOrder
https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L368-L408
The code that is responsible for paying the fee to msg.sender is following function call
https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L373-L384
Which , if we take a look at the values provided , we see the function call as follows
Here is the signature of called function
https://github.com/equilibria-xyz/root/blob/f4b832b22da8c9969e7dc5d8e9c4163f51f45ce8/contracts/attribute/Kept/Kept.sol#L99-L105
Plus the struct definition of the keeper config is as follows
So it means the function call resolves to this
Now when we pass these params to the target function , we go one level deep to calculate the output of this function
https://github.com/equilibria-xyz/root/blob/f4b832b22da8c9969e7dc5d8e9c4163f51f45ce8/contracts/attribute/Kept/Kept.sol#L99-L116
Let's take a look at calculating baseFee and calldata fee
_baseFee(0,0,some_buffer_base) -
this results in
(0x0 + keeper_buffer_base)block.baseFee
If we take a look at constructor
there is not enforcement on non-zero value of
keepBufferBase
, which means It can be zero.so out baseFee returned will be
(0x0 + 0)xBlock.baseFee = 0
So baseFee = 0
For
calldataFee
, it is rather simple , it always return zerocoming back to our handleKeeperFee function
https://github.com/equilibria-xyz/root/blob/f4b832b22da8c9969e7dc5d8e9c4163f51f45ce8/contracts/attribute/Kept/Kept.sol#L99-L116
we have baseFee = 0 , calldataFee=0
Now following lines remain
keeperFee will be
(0x0+0)xsomeEtherPrice
=>0
because applicableValue=0 , baseFee=0,calldataFee=0So now keeperFee=0
Now
_raiseKeeperFee
is called with zerokeeperFee
which will return a minimum of 0 and some non zero value, and we know the returned value will be zero
back to rest of code with keeperFee=0
zero fee will be paid to msg.sender and the event will be emitted
Impact
msg.sender is not paid the fee when the function is developed for that purpose : Being inconsistent with the Natspec and
will cause future integration issues in future.
Code Snippet
https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L368-L384
Tool used
Manual Review
Recommendation
Ensure
keepBufferBase
is non-zero in constructor.