Find file History
Pull request Compare This branch is 134 commits ahead of swim-gateway:master.
Fetching latest commit…
Cannot retrieve the latest commit at this time.
Permalink
Type Name Latest commit message Commit time
..
Failed to load latest commit information.
code-review
flattened
test
README.md

README.md

OAX Swim Gateway Stable Coin Contract Audit

Summary

OAX has developed a set of smart contracts for their swim gateway stable coins.

Bok Consulting Pty Ltd was commissioned to perform an audit on these Ethereum smart contracts.

This audit has been conducted on the source code from swim-gateway/stable-coin master-gitlab branch in commits 75cc80c, a53dce5 and daa965a.

The first round of audit was conducted on the commits above. From this audit, the following changes were made in commit a57697c.

The second round of audit was conducted on the commit above. From this audit, the following changes were made in commit cb7c991.

This report is based on the third round of audit, conducted on the commits above.

No potential vulnerabilities have been identified in the smart contracts.



Table Of Contents



Scope

This audit is into the technical aspects of the OAX swim gateway stable coin smart contracts. The primary aim of this audit is to ensure that funds stored in these contracts are not easily attacked or stolen by third parties. The secondary aim of this audit is to ensure the coded algorithms work as expected. This audit does not guarantee that that the code is bugfree, but intends to highlight any areas of weaknesses.



Risks

The permissioning for the execution of functions for this set of contracts is dependent on DSRoles (implemented in GateRoles) and DSGuard (implemented FiatTokenGuard). It it important for these permissions to be set up and maintained correctly to prevent the unauthorised execution of the smart contract functionality.



Results

The OAX Swim Gateway Stable Coin contracts have been built using the Dappsys components. There were several potential vulnerabilities identified - these have all been resolved with updates to the code. The details are listed below for each component contract.


GateRoles

GateRoles defines 3 separate roles for roles that are assigned to accounts - SYSTEM_ADMIN, KYC_OPERATOR and MONEY_OPERATOR. These roles are permissioned to execute various functions in this set of smart contracts.

It is important to set up and maintain the permissions and accounts assigned these roles to prevent unauthorised execution of the smart contract functions.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Resolved Issues

  • MEDIUM IMPORTANCE DSRoles (implemented in GateRoles) and DSGuard (implemented FiatTokenGuard) are two permissioning modules for these set of contracts, and are critical to the security of these set of contracts. While the DSGuard permission setting functions log events, the DSRoles permission setting functions do not. Search for // BK NOTE in test/modifiedContracts/dappsys.sol for the an example of the events that should be added to DSRoles to provide more visibility into the state of the permissioning

DSGuard

DSGuard provides the dappsys-style permissions for GateWithFee to execute specified functions in the FiatToken and LimitController contracts.

It is important to set up and maintain the permissions and accounts assigned these roles to prevent unauthorised execution of the smart contract functions.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Issues

None identified


AddressStatus

AddressStatus provides the contracts to maintain the blacklist and KYC-ed addresses for Membership, BaseRule, BoundaryKycRule and FullKycRule.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Issues

None identified


Membership

Membership maintains an AddressStatus list of members. Currently the MockMembership contract returns a true status for any address not defined in the membership list.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Resolved Issues

  • LOW IMPORTANCE The name MockOAXMembership implies that the contract is for testing and not to be included with the production code in Membership.sol
    • Developer has advised that the name MockOAXMembership will be used in the current form

TransferFeeController

Membership maintains an AddressStatus list of members. Currently the MockMembership contract returns a true status for any address not defined in the membership list.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Resolved Issues

  • LOW IMPORTANCE Add event to log calls to TransferFeeController.setDefaultTransferFee(...)

LimitSetting

LimitSetting restricts the number of tokens that can be minted or burnt per day.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Resolved Issues

  • LOW IMPORTANCE Could use require(...) instead of assert(...) in LimitSetting to save on gas when errored
  • LOW IMPORTANCE In LimitSetting, overloading the events AdjustMintLimitRequested(...) and AdjustBurnLimitRequested(...) makes it difficult to retrieve the events with JavaScript
  • LOW IMPORTANCE In LimitSetting.getDefaultDelayHours(), instead of using the magic number, use 30 days or a named constant
  • LOW IMPORTANCE In LimitSetting.setSettingDefaultDelayHours(...), consider adding a check that the _hours is a reasonable number
  • LOW IMPORTANCE In the LimitSetting constructor, _defaultLimitCounterResetTimeffset should be named _defaultLimitCounterResetTimeOffset
  • LOW IMPORTANCE In LimitSetting, *DefaultDelayHours* sometimes refers to hours and sometimes seconds. Consider renaming to remove ambiguity

TokenRules

TokenRules enforces the rules for the transferring, minting and burning of tokens in the BaseRule, BoundaryKycRule and FullKycRule contracts.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Resolved Issues

  • LOW IMPORTANCE Consider setting canApprove(...), canTransferFrom(...), canTransfer(...), canMint(...) and canBurn(...) to view functions - in TokenAuth.sol and TokenRules.sol

LimitController

LimitController records the daily minting and burning and checks these against the daily limits.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Resolved Issues

  • LOW IMPORTANCE Consider making LimitController.limitSetting public for traceability
  • LOW IMPORTANCE Could use require(...) instead of assert(...) in LimitController.resetLimit() to save on gas for errors

FiatToken

FiatToken is the token contract that is restricted by the TokenAuth and TransferFeeController.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Resolved Issues

  • MEDIUM IMPORTANCE For the token total supply and movements to be transparently tracked on blockchain explorers, emit Transfer(address(0), guy, wad) should be added to FiatToken.mint(...) and emit Transfer(guy, address(0), wad) should be added to FiatToken.burn(...)
  • LOW IMPORTANCE In DSToken, name() and symbol() are defined as bytes32 instead of string as specified in the ERC20 token standard. There are other token contracts using bytes32 and they are operating without problems in the blockchain explorers
    • Developer and auditor agreed that no action is required
  • LOW IMPORTANCE Consider logging events for FiatToken.setTransferFeeCollector(...) and FiatToken.setTransferFeeController(...)

GateWithFee

GateWithFee allows a gateway to mint and burn tokens within the restricted daily limits.

Potential Vulnerabilities

No potential vulnerabilities have been identified in this component.

Resolved Issues

  • MEDIUM IMPORTANCE See Notes - GateWithFee Approve And TransferFrom below
    • Alex has responded that GateWithFee will not hold any FiatToken token balance. I have added a new recommendation to add auth for both approve(...) functions
  • MEDIUM IMPORTANCE See Notes - GateWithFee Fee Accounting below
    • Kirsten has responded that the fee account flows will be accounted for in OAX's accounting reconciliation
  • LOW IMPORTANCE Gate.mint(...) currently logs an emit Transfer(0x0, guy, wad); event, but this is not required for this non-token contract as it should be tracked on the FiatToken contract. Consider renaming to Deposited(guy, wad)
  • LOW IMPORTANCE GateWithFee.transferFeeController is not used
  • LOW IMPORTANCE See Notes - GateWithFee Approve And TransferFrom below - add the auth permissioning to both the DSSoloVault.approve(...) functions, just to be sure that it will not be used by unauthorised accounts
  • LOW IMPORTANCE Consider logging events for TokenAuth:ERC20Auth.setERC20Authority(...) and TokenAuth:TokenAuth.setTokenAuthority(...)
  • LOW IMPORTANCE Consider making TokenAuth:TokenAuth.tokenAuthority public for traceability
  • LOW IMPORTANCE Could use require(...) instead of assert(...) in TokenAuth:ERC20Auth.*(...) to save on gas for errors
  • LOW IMPORTANCE Could use require(...) instead of assert(...) in TokenAuth:TokenAuth.*(...) to save on gas for errors

Notes On Resolved Issues

GateWithFee Approve And TransferFrom

MEDIUM IMPORTANCE If the GateWithFee contract has a FiatToken token balance, any KYC-ed user account can execute approve(address,uint) or approve(address), and then execute FiatToken.transferFrom(...) this token balance to the user's account. However, the MONEY_OPERATOR can freeze the user's account.

  • Alex has responded that GateWithFee will not hold any FiatToken token balance. I have added a new recommendation to add auth for both approve(...) functions
GateWithFee Fee Accounting

Reference GateWithFee.mint(...) - If there is a deposit of 1 dollar, with a 1 dollar fee, the token issuing entity will receive 2 dollars of which 1 dollar will go into a trust account and 1 dollar will go into a fee account. 2 tokens will be issued in the FiatToken contract, and backing this is the 1 dollar deposited into the trust account. The maths will not work out unless the entity's fee account balance is also reflected in the FiatToken contract balances. This also applies to the GateWithFee.burn(...) function.

  • Kirsten has responded that the fee account flows will be accounted for in OAX's accounting reconciliation

Other Resolved Issues



Code Review Of Components

Source code for the component files (first column downwards) were code-reviewed. Flattened (consolidated) versions of the contracts (columns 3 and to the right) were generated to understand the structure of the contract that will be deployed.

Code Review Contracts & Interfaces GateRoles DSGuard AddressStatus Membership TransferFeeController LimitSetting TokenRules LimitController FiatToken GateWithFee
dappsys DSMath 1 1 1 1
ERC20Events, ERC20 2 2
DSAuthority, DSAuthEvents, DSAuth 1 1 1 1 2 1 1 2 3 3
DSNote, DSStop 2 3 4 4
DSGuardEvents, DSGuard 2
DSRoles 2 5
DSTokenBase and DSToken 5 6
solovault DSSoloVault 7
GateRoles GateRoles 3 8
TokenAuth ERC20Authority, ERC20Auth, TokenAuthority, TokenAuth 2 6 9
AddressStatus AddressStatus 2 2 3
Membership MembershipInterface, MockOAXMembership 3 4
TransferFeeControllerInterface TransferFeeControllerInterface 3 7 10
FiatToken FiatToken 8 11
LimitSetting LimitSetting 3 4 12
TokenRules BaseRule, BoundaryKycRule, FullKycRule 5
LimitController LimitController 5 13
Gate Gate 14
TransferFeeController TransferFeeController 4 15
GateWithFee GateWithFee 16

Following is the list of code-reviewed contracts showing the inheritance structure:


Outside Scope

The Gnosis Multisig wallet smart contract is outside the scope of this audit, but there is a duplicated contract in the source code:



Testing

Details of the testing environment can be found in test.

../chain/index.js and ../chain/lib/deployerProd.js were used as a guide for the security model used with this set of contracts.

The following functions were tested using the script test/01_test1.sh with the summary results saved in test/test1results.txt and the detailed output saved in test/test1output.txt:

  • Group #1 deployment
    • GateRoles()
    • FiatTokenGuard()
  • Group #2 deployment
    • Blacklist"AddressStatus"(GateRoles)
    • Kyc"AddressStatus"(GateRoles)
    • MockMembership(GateRoles)
    • TransferFeeController(GateRoles, 0, 0)
    • LimitSetting(GateRoles, {mint limit}, {burn limit}, ...)
  • Group #3 deployment
    • BaseRule(Blacklist)
    • BoundaryKycRule(Blacklist, Kyc, MockMembership)
    • FullKycRule(Blacklist, Kyc, MockMembership)
    • LimitController(FiatTokenGuard, LimitSetting)
    • FiatToken(FiatTokenGuard, {symbol}, {name}, {transferFeeCollector}, TransferFeeController)
  • Set User Roles
    • GateRoles([sysAdmin=SYSTEM_ADMIN_ROLE(1), kycOperator=KYC_OPERATOR_ROLE(2), moneyOperator=MONEY_OPERATOR_ROLE(3)])
  • Set Roles Rules #1
    • GateRoles.setRoleCapability(KYC_OPERATOR_ROLE, Kyc(AddressStatus), sig("set(address,bool)"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, Blacklist(AddressStatus), sig("set(address)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, LimitSetting, sig("setDefaultDelayHours(uint256)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, LimitSetting, sig("setLimitCounterResetTimeOffset(int256)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, LimitSetting, sig("setDefaultMintDailyLimit(uint256)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, LimitSetting, sig("setDefaultBurnDailyLimit(uint256)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, LimitSetting, sig("setCustomMintDailyLimit(address,uint256)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, LimitSetting, sig("setCustomBurnDailyLimit(address,uint256)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, transferFeeController, sig("setDefaultTransferFee(uint256,uint256)"))
  • Group #4 deployment
    • GateWithFee(GateRoles, FiatToken, LimitController, {mintFeeCollector}, {burnFeeCollector}, TransferFeeController)
  • Set Roles Rules #2
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("mint(uint256)"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("mint(address,uint256)"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("burn(uint256)"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("burn(address,uint256)"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("start()"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("stop()"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("startToken()"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("stopToken()"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, GateWithFee, sig("setERC20Authority(address)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, GateWithFee, sig("setTokenAuthority(address)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, GateWithFee, sig("setLimitController(address)"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("mintWithFee(address,uint256,uint256)"))
    • GateRoles.setRoleCapability(MONEY_OPERATOR_ROLE, GateWithFee, sig("burnWithFee(address,uint256,uint256)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, GateWithFee, sig("setFeeCollector(address)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, GateWithFee, sig("setTransferFeeCollector(address)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, GateWithFee, sig("setTransferFeeController(address)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, GateWithFee, sig("setMintFeeCollector(address)"))
    • GateRoles.setRoleCapability(SYSTEM_ADMIN_ROLE, GateWithFee, sig("setBurnFeeCollector(address)"))
  • Set Guard Rules #1
    • FiatToken.permit(GateWithFee, FiatToken, sig("setName(bytes32)"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("mint(uint256)"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("mint(address,uint256)"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("burn(uint256)"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("burn(address,uint256)"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("setERC20Authority(address)"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("setTokenAuthority(address)"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("start()"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("stop()"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("setTransferFeeCollector(address)"))
    • FiatToken.permit(GateWithFee, FiatToken, sig("setTransferFeeController(address)"))
    • FiatToken.permit(GateWithFee, LimitController, sig("bumpMintLimitCounter(uint256)"))
    • FiatToken.permit(GateWithFee, LimitController, sig("bumpBurnLimitCounter(uint256)"))


(c) BokkyPooBah / Bok Consulting Pty Ltd for OAX - Sep 26 2018. Done with assistance from Adrian Guerrera. The MIT Licence.