Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Low Issues

bauchibred

L-01 tillIn() expects msg.value but settleAskMaker() never passes msg.value to it

Summary

In some cases, when querying and this check always reverts the query when the token is the wrapped native and no msg.value was attached.

However when calling this frunction from settleAskMaker() in no instance is msg.value attached which causes settling to not be possible with native eth, since we can't pass eth, see https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L267-L272

tokenManager.tillIn(
_msgSender(),
marketPlaceInfo.tokenAddress,
settledPointTokenAmount,
true
);

Impact

tillIn() would not work with settleAskMaker() since it never passes msg.value to it.

NB: A similar bug case is applicable here too https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/DeliveryPlace.sol#L377-L383

tokenManager.tillIn(
_msgSender(),
marketPlaceInfo.tokenAddress,
settledPointTokenAmount,
true
);
​

Tools Used

Manual review

Recommendations

Consider remodifying this and in a case where the token is native eth, pass msg.value.

L-02 _safe_transfer & _safe_transfer_from would never work native ether

Summary

_safe_transfer & _safe_transfer_from would always fail for native ether

Vulnerability Details

See https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/utils/Rescuable.sol#L78-L117

/**
* @dev Safe transfer.
* @param token The token to transfer. If 0, it is ether.
* @param to The address of the account to transfer to.
* @param amount The amount to transfer.
*/
function _safe_transfer(
address token,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}
​
/**
* @dev Safe transfer.
* @param token The token to transfer. If 0, it is ether.
* @param to The address of the account to transfer to.
* @param amount The amount to transfer.
*/
function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}

Per the documentation and the comments accompanying the functions there is an intention to include native eth transfer within these functions, and as the comments suggest in the case where the token specified is 0x0 then the token is native ether and the transfer is expected to be processed as is done in here in rescue(). However in our case payable.transfer() is never used to tramsfer out the eth, so the eth transfers via these two functions never work considering we attempt to tranfer a 0x0 token instead,

Impact

_safe_transfer & _safe_transfer_from would never work native ether.

NB: Attaching as low as this could be intended and there is just an error with the comments.

Tools Used

Manual review

Recommendations

Consider checking if token is 0 and then use a method that can tramsfer out native eth instead.

L-03 Lightchaser missed instance in rescue()

Summary

NB: This instance has not been caught by Light Chaser which is why I'm submitting this report.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/utils/Rescuable.sol#L64-L76

function rescue(
address to,
address token,
uint256 amount
) external onlyOwner {
if (token == address(0x0)) {
payable(to).transfer(amount);
} else {
_safe_transfer(token, to, amount);
}
​
emit Rescue(to, token, amount);
}

This function is used to rescue tokens ann depending on if the token is native eth or not it uses different methods in transferring out the tokens, issue however is that it uses transfer to send Ether, which can fail if the recipient is a contract with a fallback function that uses more than 2300 gas.

Impact

Tokens would be unrescuable to some accounts, considering the attempt always reverts here.

Tools Used

Manual review

Recommendations

Consider using the .call() method instead.

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-PreMarkets-settleAskMaker-settleAskTaker-no-msg.value-sent

Invalid, in `settleAskMaker` and `settleAskTaker` you are settling the point token to be given to the takers, which is an ERC20 token, so no native ETH is involved and thus no msg.value is required.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.