Kickback Audit

The Kickback team asked us to review the Kickback contracts. We looked at the code and now publish our results.

The audited commit is 587d658bde239fdbbe8efef35f9ba9a0e172fc88 and all solidity files were included in the scope.

This audit was a full audit covering the entire project. The audit encompassed a deep dive into the system as a whole and assessments of the individual components. Additionally, the auditors reviewed many different cases that may have led to exploits or bugs in the code.

Here are our assessment and recommendations, in order of importance.

Issues

Critical

None

High

None

Medium

[M01] Limit of participants should not be lower than existing participants

The limitOfParticipants variable can be edited during an event and can be changed to a value that is less than the number of currently registered participants.

Consider either only allowing an admin to call this function if noOneRegistered is true or if a check is put in place such that the new _limitOfParticipants is greater than or equal to the number of registered users.

Update: Fixed in 6280641

Low

[L01] admins array should be a mapping

As it stands, the admins array in GroupAdmin.sol keeps track of which addresses are admins. Because it is an array, code has been added to deal with various reads and writes of this array that may not be necessary if admins were a mapping instead of an array.

Consider rearchitecting GroupAdmin.sol to make admins a mapping instead of an array. Additionally, a counter can be incremented and decremented when an admin is added or removed so that the numOfAdmins() function will still return a valid number.

By doing this, the revoke() and isAdmin() functions would become much simpler and more gas efficient. getAdmins() would no longer be a valid function and would have to be tracked off-chain with events (the getAdmins() function can also be tracked off-chain, if desired, since it is unused elsewhere in the project)

Additionally, this allows for the resolution of the grant() should not push existing admins issue.

As a note, this new off-chain architecture is only possible if the grant() should not push existing admins suggestion is resolved.

Update: The Kickback team plans to fix this in a future iteration of the contracts. This issue does not present an immediate security risk.

[L02] Functions left out of the interface

The Conference interface is missing the changeDeposit() and participantsIndex() function signatures. Consider adding them to the interface.

Update: Fixed in 7cf134f and 1b5f885

[L03] grant() should not push existing admins

The grant() function currently pushes admins to the admin array and emits an AdminGranted event even if duplicate admins are passed into the function. This may add confusion to any interface that is using events as a way to read the contract.

Consider following the suggestions in the admins array should be a mapping issue and then skipping the addition and event emission if an address is already an admin.

Update: The Kickback team plans to fix this in a future iteration of the contracts. This issue does not present an immediate security risk.

[L04] Revert when more attendees than registered

On line 214 of the AbstractConference contract, totalAttended is set to registered if there were more attendees counted than were registered. Because this indicates the input parameters were erroneous, the transaction should revert if more attendees were counted than were registered.

Update: Fixed in 0f851a4

[L05] Include owner in getAdmins() and numOfAdmins()

The GroupAdmin contract has a concept of both an owner and an admin. The owner is never explicitly set as an admin, but is allowed to perform admin duties because of this line. Because of this, the getAdmins() and numOfAdmins() functions do not reflect the fact that the owner is an admin.

Consider including the owner in both the getAdmins() and numOfAdmins() functions.

Update: The Kickback team plans to fix this in a future iteration of the contracts. This issue does not present an immediate security risk.

[L06] Admin Denial of Service Attack

An admin is able to DoS an event contract by continuously adding admins by calling the grant function continuously. If enough admins are added, then any function in AbstractConference.sol with an onlyAdmin modifier may run out of gas if called by certain admins.

This is a low severity issue because it would require a malicious admin to add a very large number of admins. The economic cost to DoS the contract would be incredibly high and would likely not result in any financial gain.

Update: The Kickback team plans to fix this in a future iteration of the contracts. This issue does not present an immediate security risk.

[L07] ETH Can be Stuck in DAI Party contract

ETH can be sent to a party contract that is meant to only accept DAI. This is caused by the fact that register is a payable function and no checks are put in place to stop ETH from being sent to a DAI contract.

Consider adding require(msg.value == 0); to the doDeposit() function of ERC20Conference.sol.

Update: The Kickback team discovered this issue during the audit bug fixes. The issue has been resolved with 3dbf0f1.

Notes

• The constructor in AbstractConference.sol is missing a docstring for the _owner parameter.
  Update: Fixed in 7cf134f
• There are a few misspellings throughout the code. The following are some locations where this was noticed and should be fixed:
  - @dev Returns number of admings
  - @return Number of admings
  - Each revert statement in AbstractConference.sol has "must be implemented int the child class".
  - ower address is required
  Update: Fixed in 7cf134f
• The isAdmin() function checks if the passed in parameter is an admin. Because the address may or may not be an admin when being passed in to this function, consider renaming the parameter to potentialAdmin.
• The purpose of some variables in AbstractConference.sol are not clear based on their names. Consider renaming the following variables to more accurately describe their purpose:
  -registered to numRegistered
-ended to isEnded
-cancelled to isCancelled
-Participant.paid to Participant.paidOut
• The value declaration is hard to read and is error prone. Consider using a built-in constant such as ether or scientific notation to declare this value.
  Update: Fixed in 7cf134f
• The owner of an event has full control of the funds held by the event contract is able to distribute them in any way they see fit. On-chain dispute and arbitration methods are currently being researched to remove the need for participants to trust the event organizer.

Conclusion

No critical or high issues were found. Some changes were proposed to follow best practices and reduce the potential attack surface. Overall, the code was well written and void of any serious flaws.