Smart Contracts – Code Review for Smart Contracts to Pay Beneficiaries

contract-designinternal-transactionssolidity

I wrote two smart contracts to create a "smart asset". The idea is that beneficiaries involved in the asset are payed the usage price of the asset once it is used. So for example, four parties (designers, raw material delivers, maintainers, and the assemblers) create a washing machine, then they create a smart contract that pays the beneficiaries once a consumer uses the washing machine. (Idea is to go from an economy of waste, to an economy of usage, with an incentive to create sustainable products.)

The SimpleSmartAssetManager is the smart contract that instantiates smart assets. And the SimpleSmartAsset is a representation of the smart asset.

I made it so that a beneficiary consists of an address and a weight. The weight is the relative weight of the usage price the beneficiary receives on usage of the asset. When the asset is used (by calling pay() on the SimpleSmartAsset), the price for usage will be deducted from the user, and the added beneficiaries will be paid their relative weight (their weight as percentage of total weight).

The SimpleSmartAssetManager has a list of existing smart assets. When a smart asset is used the manager retrieves the address of the specific instance of the SimpleSmartAsset and calls the pay function. All calls should go through the the SimpleSmartAssetManager, the owner of the contract.

This is the UML contract-diagram:

UML

When functions are called some events are logged to the blockchain.

Mortal and Owned contracts (so contracts have the onlyOwner modifier and can be removed):

pragma solidity ^0.4.10;


contract Owned {

  address owner;

  modifier onlyOwner {
    require(msg.sender == owner);
    _;
  }
}


contract Mortal is Owned {
  function remove() onlyOwner {
    selfdestruct(owner);
  }
}

the SimpleSmartAsset:

contract SimpleSmartAsset is Mortal {

  uint usagePrice;
  Beneficiary[] beneficiaries;
  uint totalWeight; // to calculate percentage the beneficiaries receive

  event AssetCreated(uint _usagePrice,
                     address[] addresses,
                     uint[] weights);

  function SimpleSmartAsset(uint _usagePrice,
                            address[] addresses,
                            uint[] weights) {
    owner = msg.sender;
    usagePrice = _usagePrice;

    uint beneficiaryCount = addresses.length;
    for (uint i = 0; i < beneficiaryCount; i++) {

      uint weight = weights[i];

      addBeneficiary(addresses[i], weight);
      totalWeight += weight;
    }

    AssetCreated(_usagePrice, addresses, weights);
  }

  function getUsagePrice() constant returns (uint) {
    return usagePrice;
  }

  event BeneficiaryPaid(address addr, uint amount);

  function pay() payable onlyOwner {
    require(msg.value >= usagePrice);

    uint beneficiaryCount = beneficiaries.length;
    for (uint i = 0; i < beneficiaryCount; i++) {

      Beneficiary memory beneficiary = beneficiaries[i];

      uint weight = beneficiary.weight;
      address addr = beneficiary.addr;

      uint amount = (weight * usagePrice) / totalWeight;

      addr.transfer(amount);
      BeneficiaryPaid(addr, amount);
    }
  }

  struct Beneficiary {
    address addr;
    uint weight;
  }

  function addBeneficiary(address addr, uint weight) onlyOwner {
    beneficiaries.push(Beneficiary({
        addr: addr,
        weight: weight
    }));
  }

}

and the SimpleSmartAssetManager:

contract SimpleSmartAssetManager is Mortal {

  mapping(string => address) smartAssets;

  function SimpleSmartAssetManager() {
    owner = msg.sender;
  }

  function createSmartAsset(string name,
                            uint usagePrice,
                            address[] addresses,
                            uint[] weights) {

    require(addresses.length == weights.length);
    require(smartAssets[name] == address(0x0));

    address assetAddress = new SimpleSmartAsset(usagePrice,
                                                addresses,
                                                weights);
    smartAssets[name] = assetAddress;
  }

  function getUsagePrice(string assetName)
    constant returns (uint) {
    address assetAddress = smartAssets[assetName];
    uint price = getUsagePrice(assetAddress);

    return price;
  }

  function getUsagePrice(address assetAddress)
    constant returns (uint) {
    uint price = SimpleSmartAsset(assetAddress).getUsagePrice();

    return price;
  }

  event AssetUsed(string name, uint usagePrice);

  function useAsset(string name) payable {
    address assetAddress = smartAssets[name];
    uint price = getUsagePrice(assetAddress);

    require (msg.value >= price);

    SimpleSmartAsset(assetAddress).pay.value(msg.value)();

    AssetUsed(name, price);
  }

  function removeAsset(string name) onlyOwner {
    address assetAddress = smartAssets[name];
    SimpleSmartAsset(assetAddress).remove();
  }
}

These were the first contracts I wrote, I am curious how I can improve.

Best Answer

Nice contracts as for a beginner, well done!

First two problems that I've noticed after reading your code:

1.You are using an unsafe method in looping through beneficiaries:

addr.transfer(amount);

so if one user throws while accepting a transfer it could freeze payments for the others. Consider using pull payments pattern.

2.You are allowing a theoretically infinite number of beneficiaries:

function addBeneficiary(address addr, uint weight) onlyOwner {
    beneficiaries.push(Beneficiary({
        addr: addr,
        weight: weight
    }));
  }

For safety reasons, I would check how many beneficiaries are possible to serve without breaking the gas limit and I'd suggest putting a hard cap on the number of beneficiaries. Without it, you are risking freezing your assets as the loop may never execute completely.

Related Topic