Avoid passing through msg.value within Miner API

All Miner calls pass msg.value as provided value (example here).

This is a path that we should probably avoid. We should keep in mind that msg.value within an internal function is NOT a value passed to that internal function. msg.value is constant within a call stack frame.

The consumer of the Miner API isn’t signaling the intent to transfer value just because msg.value is nonzero - the Miner API methods might be getting called as part of a higher-level routine, part of which entails accepting and handling value.

In the example linked above, we’re implicitly sending value to a call to Miner::get_owner. That’s unlikely to be what the user wants. Here’s an example of a higher-level routine where this would result in unwanted behavior:

function transferFundsToOwner(bytes memory _miner) public payable { MinerTypes.GetOwnerReturn memory ret = _miner.getOwner(); ret.owner.call{ value: msg.value }(""); } The above function is meant to accept Fil via callvalue, query the owner of a miner address, and transfer the callvalue to said owner. However, because getOwner forwards msg.value to Actor.call, this function actually ends up sending the call value to the Miner contract, rather than the Miner owner.

Recommendations:

Never send value to methods like getOwner - getOwner should be a read-only getter, not a way to transfer funds.
If any methods in MinerAPI are going to send value, don’t implicitly forward msg.value, add a parameter that allows the API consumer to specify the amount to send.
It’s probably a good idea to add an explicit “send value” method, rather than tacking this functionality onto other Miner methods.

This issue is created as a result of an audit we did for the library.

Zondax / filecoin-solidity

Avoid passing through msg.value within Miner API #195