search

code-423n4 / 2022-04-abranft-findings

0 stars 0 forks source link

Any non-native asset on the NFTPair or NFTPairWithOracle balance except collateral is a free grab #164

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L622-L624 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L674-L675

Vulnerability details

Impact

Any asset that is non-native (some contract controlled) and is not collateral can be stoled from the NFTPair and NFTPairWithOracle contracts' balances.

For example, a malicious user can setup a bot that immediately transfers away all mistakenly sent funds from NFTPair / NFTPairWithOracle, so a user will not have a chance to retrieve them.

On the one hand the contract aren't expected to hold non-collateral assets, on the other that's a clear funds loss case that can be fixed with additional controls, so setting the severity to medium.

Proof of Concept

_call() accepts arbitrary callee address and call arguments from a user, them performs the call on the user behalf:

require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");

(bool success, bytes memory returnData) = callee.call{value: value}(callData);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L622-L624

_call() can be invoked with cook(), which passes the above arguments as is from the user:

if (action == ACTION_CALL) {
    (bytes memory returnData, uint8 returnValues) = _call(values[i], datas[i], value1, value2);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L674-L675

This way any non-native and not collateral asset that NFTPair owns can be asked to be transferred to attacker's account.

Recommended Mitigation Steps

Consider the possibility to introduce the function white list that contains only relevant functions for ACTION_CALL. That will keep the usability, greatly reducing described and all related attack surfaces.

cryptolyndon commented 2 years ago

This is not a bug.

The only way to prevent ALL tokens from being taken, is to, for all intents and purposes, get rid of cook() and skimming.

How would you deal with some future token standard? Tokens other than the asset in today's BentoBox? Tokens staked somewhere in the name of the contract?

0xean commented 2 years ago

closing as invalid. A user sending funds to any address they don't control or understand will always result in them losing funds. This is not unique to this contract in any way.