Report generated by auditbase.com
This report includes two parts:
The function fulfill sets a MemID redeeming status to true even if it's already marked as redeemed even after asserting it was not in validateUnlock.
require(!midIsRedeemed[memid], "err_mid_redeemed");
// ...
midIsRedeemed[memid] = true;Confidence: 0.9
Severity: High
The midIsRedeemed mapping is utilized to keep track of whether a particular MEM ID has been used (redeemed) or not.
The validateUnlock and executeUnlock functions both set this mapping to true for the same _memid, which appears redundant and opens up potential vulnerabilities.
validateUnlock can potentially set midIsRedeemed[_memid] to true prematurely: In validateUnlock, the mapping is set to true right after the Chainlink oracle request is made, before actually validating or fulfilling the amount that is to be unlocked. This could be a design choice to prevent double spending but it also means that if the Chainlink oracle call fails or is delayed, the MEM ID will have been marked as redeemed when no tokens have actually been unlocked.
// validateUnlock function snippet
function validateUnlock(string calldata _memid) public returns (bytes32 requestId) {
// ...
midIsRedeemed[_memid] = true; // premature flag setting
// ...
}
Confidence: 0.9
Severity: High (could lead to funds being locked with no way to unlock)
The contract relies on external validation (validateUnlock) that a MemID can be unlocked. If the external service is compromised, incorrect validation could lead to unauthorized unlocking of funds.
Chainlink.Request memory req = _buildOperatorRequest(jobId, this.fulfill.selector);https://github.com/fatherGoose1/ai-competition/blob/main/src/bridge.sol#L96-L99
Confidence: 0.8
Severity: High
Having a single EOA as the only owner of contracts is a large centralization risk and a single point of failure. A single private key may be taken in a hack, or the sole holder of the key may become unable to retrieve the key when necessary. Consider changing to a multi-signature setup, or having a role-based authorization model.
214 function withdrawLink() public onlyOwner {
240 function setOracleAddress(address _oracleAddress) public onlyOwner {
247 function getOracleAddress() public view onlyOwner returns (address) {
254 function setJobId(string memory _jobId) public onlyOwner {
260 function getJobId() public view onlyOwner returns (string memory) {
267 function setFeeInJuels(uint256 _feeInJuels) public onlyOwner {
278 ) public onlyOwner {
287 onlyOwner
Example: https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers.
158 token.safeTransferFrom(msg.sender, address(this), _amount);
200 token.safeTransfer(msg.sender, net_amount);
217 link.transfer(msg.sender, link.balanceOf(address(this))),
229 token.safeTransfer(treasury, amount);
Some IERC20 implementations (e.g UNI, COMP) may fail if the valued transferred is larger than uint96. Source
158 token.safeTransferFrom(msg.sender, address(this), _amount);
200 token.safeTransfer(msg.sender, net_amount);
229 token.safeTransfer(treasury, amount);
Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator.
290 return (oracleFee * 100) / LINK_DIVISIBILITY;
Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that 'The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix'.
94 assert(!midIsRedeemed[_memid]);
227 assert(amount > 0);
This issue arises when an address state variable is assigned a value without a preceding check to ensure it isn't address(0x0). This can lead to unexpected behavior as address(0x0) often represents an uninitialized address.
241 oracleAddress = _oracleAddress;
Setters should have initial value check to prevent assigning wrong value to the variable. Assginment of wrong value can lead to unexpected behavior of the contract.
240 function setOracleAddress(address _oracleAddress) public onlyOwner {
241 oracleAddress = _oracleAddress;
242 _setChainlinkOracle(_oracleAddress);
243 }
254 function setJobId(string memory _jobId) public onlyOwner {
255 jobId = bytes32(bytes(_jobId));
256 }
267 function setFeeInJuels(uint256 _feeInJuels) public onlyOwner {
268 oracleFee = _feeInJuels;
269 }
276 function setFeeInHundredthsOfLink(
277 uint256 _feeInHundredthsOfLink
278 ) public onlyOwner {
279 setFeeInJuels((_feeInHundredthsOfLink * LINK_DIVISIBILITY) / 100);
280 }
It is important to ensure state variables numbers are set to a reasonable value.
82 bridgeFee = _bfee; // 0.25% for the launch so uint256(25)
268 oracleFee = _feeInJuels;
Fees/rates should be required to be below 100%, preferably at a much lower limit, to ensure users don't have to monitor the blockchain for changes prior to using the protocol.
267 function setFeeInJuels(uint256 _feeInJuels) public onlyOwner {
268 oracleFee = _feeInJuels;
269 }
276 function setFeeInHundredthsOfLink(
277 uint256 _feeInHundredthsOfLink
278 ) public onlyOwner {
279 setFeeInJuels((_feeInHundredthsOfLink * LINK_DIVISIBILITY) / 100);
280 }
Double type casting should be avoided in Solidity contracts to prevent unintended consequences and ensure accurate data representation. Performing multiple type casts in succession can lead to unexpected truncation, rounding errors, or loss of precision, potentially compromising the contract's functionality and reliability. Furthermore, double type casting can make the code less readable and harder to maintain, increasing the likelihood of errors and misunderstandings during development and debugging. To ensure precise and consistent data handling, developers should use appropriate data types and avoid unnecessary or excessive type casting, promoting a more robust and dependable contract execution.
Strings.toHexString(uint256(uint160(msg.sender)), 20)
In Solidity, when values are being assigned in constructors to unsigned or integer variables, it's crucial to ensure the provided values adhere to the protocol's specific operational boundaries as laid out in the project specifications and documentation. If the constructors lack appropriate validation checks, there's a risk of setting state variables with values that could cause unexpected and potentially detrimental behavior within the contract's operations, violating the intended logic of the protocol. This can compromise the contract's security and impact the maintainability and reliability of the system. In order to avoid such issues, it is recommended to incorporate rigorous validation checks in constructors. These checks should align with the project's defined rules and constraints, making use of Solidity's built-in require function to enforce these conditions. If the validation checks fail, the require function will cause the transaction to revert, ensuring the integrity and adherence to the protocol's expected behavior.
67 constructor(
68 IERC20 _btoken,
69 address _oracleAddress,
70 address _linkTokenAddr,
71 address _treasury,
72 string memory _jobId,
73 uint256 _ofee,
74 uint256 _bfee
75 ) ConfirmedOwner(msg.sender) {
76 token = _btoken; // 0x779877A7B0D9E8603169DdbD7836e478b4624789 $LINK
77 treasury = _treasury; // 0x747D50C93e6821277805a2B80FE9CBF72EFCe6Cd
78 _setChainlinkToken(_linkTokenAddr); // 0x779877A7B0D9E8603169DdbD7836e478b4624789
79 _setChainlinkOracle(_oracleAddress); // 0x0FaCf846af22BCE1C7f88D1d55A038F27747eD2B
80 setJobId(_jobId); // "a8356f48569c434eaa4ac5fcb4db5cc0"
81 setFeeInHundredthsOfLink(_ofee); // sepolia is zero $LINK fee
82 bridgeFee = _bfee; // 0.25% for the launch so uint256(25)
83 }
The non-upgradeable standard version of OpenZeppelin’s library is inherited/used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behavior.
Use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts where applicable. See https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/tree/master/contracts for a list of available upgradeable contracts
8 import {Strings} from "../lib/openzeppelin/contracts/utils/Strings.sol";
9 import "../lib/openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions. See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure
240 function setOracleAddress(address _oracleAddress) public onlyOwner {
77 treasury = _treasury; // 0x747D50C93e6821277805a2B80FE9CBF72EFCe6Cd
Governance functions (such as upgrading contracts, setting critical parameters) should be controlled using time locks to introduce a delay between a proposal and its execution. This gives users time to exit before a potentially dangerous or malicious operation is applied.
Click to show 8 findings
254 function setJobId(string memory _jobId) public onlyOwner {
276 function setFeeInHundredthsOfLink(
277 uint256 _feeInHundredthsOfLink
278 ) public onlyOwner {
267 function setFeeInJuels(uint256 _feeInJuels) public onlyOwner {
247 function getOracleAddress() public view onlyOwner returns (address) {
260 function getJobId() public view onlyOwner returns (string memory) {
214 function withdrawLink() public onlyOwner {a
284 function getFeeInHundredthsOfLink()
285 public
286 view
287 onlyOwner
288 returns (uint256)
289 {
240 function setOracleAddress(address _oracleAddress) public onlyOwner {
issing checks for address(0x0) when updating address state variables
77 treasury = _treasury; // 0x747D50C93e6821277805a2B80FE9CBF72EFCe6Cd
241 oracleAddress = _oracleAddress;