feat(lint): add arbitrary-send-eth lint#14943
Conversation
mablr
left a comment
There was a problem hiding this comment.
Some clanker findings, I've been able to repro locally.
FN-1 — Numeric address literals accepted as safe destinations
Source: arbitrary_send_eth.rs#L246
The destination safety check matches LitKind::Address(_) | LitKind::Number(_). Any address(N) expression resolves to a LitKind::Number node and is silently accepted as safe — the lint never warns. Only address(0) and checksummed address literals are genuinely safe destinations; arbitrary numeric casts are not.
payable(address(1)).transfer(amt); // no warning emitted — should warnFP-1 — Trailing return; disqualifies a valid caller-restriction helper
Source: arbitrary_send_eth.rs#L827
function_no_arg_returns rejects any helper body that contains a return statement, including a no-op trailing return;. A helper like if (msg.sender != owner) revert(); return; is semantically identical to the same function without the final statement, but the modifier is not recognised as caller-restricting and the transfer is falsely flagged.
function _checkOwner() internal view {
if (msg.sender != owner) revert();
return; // ← causes the whole helper to be ignored
}
modifier onlyOwner() { _checkOwner(); _; }
function withdraw(address payable to, uint256 amt) external onlyOwner {
to.transfer(amt); // falsely flagged
}FP-2 — Named return variable not recognised as a msg.sender-returning helper
Source: arbitrary_send_eth.rs#L800
function_no_arg_returns only matches StmtKind::Return(Some(e)) as the sole statement. A helper that assigns the named return variable instead of using an explicit return expression produces a different HIR shape and is not recognised, so the modifier is ignored and the transfer is falsely flagged.
function _msgSender() internal view returns (address r) { r = msg.sender; }
modifier onlyOwner() { require(_msgSender() == owner); _; }
function withdraw(address payable to, uint256 amt) external onlyOwner {
to.transfer(amt); // falsely flagged
}…ing return, named-return helpers)
…nters, allow trailing bare return
| let arg_map: Vec<(hir::VariableId, hir::VariableId)> = invocation | ||
| .args | ||
| .exprs() | ||
| .enumerate() | ||
| .filter_map(|(i, arg)| Some((*modifier.parameters.get(i)?, underlying_var(arg)?))) | ||
| .collect(); |
There was a problem hiding this comment.
FN — Named modifier arguments mapped positionally, can mark wrong destination safe
collect_modifier_safety builds arg_map by zipping invocation.args.exprs() with modifier.parameters by index. When the modifier is invoked with named args whose source order differs from the parameter order, the safety fact recovered from the modifier prefix is bound to the wrong call-site variable — the lint silently no-warns on a genuinely arbitrary destination.
arg_for_param (L1201–L1214) already handles the named-vs-positional distinction and is used elsewhere in this file; it should be reused here.
modifier check(address payable who, address payable ignored) {
require(who == payable(msg.sender));
_;
}
function withdraw(address payable to, address payable other, uint256 amt)
external
check({ignored: to, who: other}) // guard actually constrains `other`
{
to.transfer(amt); // not flagged — `to` is incorrectly marked safe
}Suggested fix:
| let arg_map: Vec<(hir::VariableId, hir::VariableId)> = invocation | |
| .args | |
| .exprs() | |
| .enumerate() | |
| .filter_map(|(i, arg)| Some((*modifier.parameters.get(i)?, underlying_var(arg)?))) | |
| .collect(); | |
| let arg_map: Vec<(hir::VariableId, hir::VariableId)> = modifier | |
| .parameters | |
| .iter() | |
| .filter_map(|&mp| { | |
| let arg = arg_for_param(hir, modifier, mp, &invocation.args)?; | |
| Some((mp, underlying_var(arg)?)) | |
| }) | |
| .collect(); |
Worth adding a regression test in ArbitrarySendEth.sol for a modifier invoked with named args in non-source order.
Flags functions that send ETH to a caller-controlled destination without restricting who can call them.