GOAL: Make solidity_coding_challenge.sol
a secure and tidy contract.
- Prefer 4 space indents (Solidity style guide)
- Prefer more explicit types ex:
uint
->uint256
- Specify visibility on functions
- Added
destroy
function. This may come in handy especially if the the contract is deployed with bugs or undesired behavior FailedSend
event did not specify names for the 2 parameters that it takes.- Added more events for successful actions and removed the
FailedSend
event. Prefering events that are emmited for important state changes, not failures. - Prefer
.transfer
over.send
so that failed sends will be reverted automatically. - Create a
Shareholder
struct in order to support theonlyShareholder
modifier. - Don't include whitespace between function (
function () payable
) and parenthesis (Solidity style guide)
Constructor function of the contract. The constructor function is optional and is only run once when the contract is created.
Bugs:
- Constructor spelling error
InseceureAndMessy
- Consequence: Constructor would not be invoked when contract is created. It would leave the function exposed to malicious parties.
- Solution: Rename the constructor to match the contract name.
Fallback function of the contract. The contract can only have one unnamed function that does not take arguments or return anything.
Bugs:
shares[msg.sender] = msg.value
resets the value of shares on every payment- Consequence: If a shareholder sends multiple payments, the record of their payments will be reset every time instead of accumulating.
- Solution: Change the operator from
=
to+=
to make sure payments are accumulated.
- No access restriction
- Consequence: Any address can send payments to this contract, even if they aren't in the
shareholders
array orshares
mapping. - Solution: Use the
onlyShareholder
modifier to protect the function.
- Consequence: Any address can send payments to this contract, even if they aren't in the
Function that allows the contract owner to add and enable new address to be shareholders
Bugs:
- Use
msg.sender
instead oftx.origin
for authorization- Consequence:
tx.origin
carries the original address that started the transaction. Meaning that if you're theowner
address and you send a transaction through a malicious wallet, it would still authorize with your address. - Solution: Use
msg.sender
instead.msg.sender
will carry the address of the immediate sender. In the case of a transaction being sent through a malicious wallet, it would fail authorization because the wallet contract address is not theowner
.
- Consequence:
- Not checking if shareholder exist before adding them.
- Consequence: This could result in repeat shareholder entries and potential loss of funds.
- Solution: Check if the shareholder
isAuthorized
before running the function
- Not adding the new shareholder to the
shares
mapping, but adding them to theshareholders
array- Consequence: This could result in mismatched shareholder entries and inability for shareholders to deposit or withdraw.
- Solution: Add an new item to the
shares
mapping, matching the newshareholders
address.
Function that allows an authorized shareholder to withdraw their funds from the contract
Bugs:
- This function in vulnerable to a re-entrancy attack
- Consequence: The recipient of the transfer could be a contract that calls back into this function. This would allow the attacker to get multiple withdrawals until the gas runs out.
- Solution: Make sure the sender has sufficient funds and discount the tokens before the transfer is called. This way, any re-entrency will fail to pull out more tokens than the shareholder has contributed.
Function that allows the contract owner to dispense all of the shares (funds) back to the shareholders.
Bugs:
- The for loop uses
var
type deduction, which could potentially cause an infinite loop.- Consequence: Since the type of
i
will be deduced to type uint8 on the first iteration and ifshareholders.length
is greater than 255, the loop would become infinite. In this case, the loop would consume all of the gas and the transaction would be terminated. - Solution: Set
i
to a type of uint256
- Consequence: Since the type of
- The success and failure cases of
shareholder.send(sh);
need to be handled properly.- Consequence: Since
shares[shareholder]
always gets set to 0, failures to send will still reset the shareholders shares to 0. - Solution: Set up success and failure handling, checking to make sure the
shareAmount
is greater than 0 and that the_shareholder.send(shareAmount)
was successful.
- Consequence: Since