Platform: Code4rena
Start Date: 09/12/2021
Pot Size: $50,000 USDC
Total HM: 19
Participants: 21
Period: 7 days
Judge: 0xean
Total Solo HM: 14
Id: 61
League: ETH
Rank: 10/21
Findings: 2
Award: $1,535.14
🌟 Selected for report: 3
🚀 Solo Findings: 1
🌟 Selected for report: gpersoon
845.0261 USDC - $845.03
gpersoon
After a master calls unlinkAddress() to unlink an address, the address that has just been unlinked can directly link again without permission. The address that is just unlinked can call linkAddress(masterAddress) which will execute because pendingLinkAddresses is still set. Assuming the master has unlinked for a good reason it is unwanted to be able to be linked again without any permission from the master.
Note: a master can prevent this by calling cancelAddressLinkingRequest(), but this doesn't seem logical to do
function unlinkAddress(address _linkedAddress) external { address _linkedTo = linkedAddresses[_linkedAddress].masterAddress; require(_linkedTo != address(0), 'V:UA-Address not linked'); require(_linkedTo == msg.sender, 'V:UA-Not linked to sender'); delete linkedAddresses[_linkedAddress]; ... } function linkAddress(address _masterAddress) external { require(linkedAddresses[msg.sender].masterAddress == address(0), 'V:LA-Address already linked'); // == true (after unlinkAddress) require(pendingLinkAddresses[msg.sender][_masterAddress], 'V:LA-No pending request'); // == true (after unlinkAddress) _linkAddress(msg.sender, _masterAddress); // // pendingLinkAddresses not reset } function cancelAddressLinkingRequest(address _linkedAddress) external { ... delete pendingLinkAddresses[_linkedAddress][msg.sender]; // only location where pendingLinkAddresses is reset
Add something like to following at the end of linkAddress:
delete pendingLinkAddresses[msg.sender][_masterAddress];
gpersoon
Suppose pool tokens are transferred from a user to himself. Then _beforeTokenTransfer will be called with _from == _to. This mostly executes as expected, with the following exception:
lenders[_from].effectiveInterestWithdrawn = (_fromBalance.sub(_amount)).mul(_totalRepaidAmount).div(_totalSupply); lenders[_to].effectiveInterestWithdrawn = (_toBalance.add(_amount)).mul(_totalRepaidAmount).div(_totalSupply);
When from== to then lenders[_from].effectiveInterestWithdrawn is overwritten when setting lenders[_to].effectiveInterestWithdrawn This will lead to the value of effectiveInterestWithdrawn not being as expected, which could have negative results.
function _beforeTokenTransfer( address _from, address _to, .....) { ... lenders[_from].effectiveInterestWithdrawn = (_fromBalance.sub(_amount)).mul(_totalRepaidAmount).div(_totalSupply); lenders[_to].effectiveInterestWithdrawn = (_toBalance.add(_amount)).mul(_totalRepaidAmount).div(_totalSupply); ...
Add something like the following in function _beforeTokenTransfer() assert( _from != _to,"Same");
#0 - ritik99
2021-12-27T05:18:51Z
Duplicate of #146
🌟 Selected for report: gpersoon
281.6754 USDC - $281.68
gpersoon
The function repayPrincipal() calls _repay() with MAX_INT as parameter. In _repay() this value (_amount) is multiplied by 10**30. As _amount already has the maximum value of an int256 it will overflow. Because solidity 7.6.0 is used and mul() isn't used (!) this actually works. The resulting value is still large and thus the function repayPrincipal() does still work.
It is not recommended to rely on overflow working and when moving to solidity 0.8.x this will no longer work.
uint256 constant MAX_INT = 2**256 - 1;
function repayPrincipal(address payable _poolID) external payable nonReentrant isPoolInitialized(_poolID) { .... uint256 _interestToRepay = _repay(_poolID, MAX_INT, true); ... function _repay( ... uint256 _amount,..) internal returns (uint256) { .. _amount = _amount * 10**30;
Use safemath in _replay() and change MAX_INT to something like:
uint256 constant LARGE_INT = 2**128
Note: 1030 ~ 2100
🌟 Selected for report: gpersoon
281.6754 USDC - $281.68
gpersoon
The function transferTokens of SavingsAccountUtil.sol sends the excess ETH to msg.sender, while a _from parameter is also present in the function. It seems more logical to send it to _from, like the similar function _transferTokens of Repayments.sol
Luckily in the current code the _from is always msg.sender so it doesn't pose a direct risk. However if the code is reused or forked it might lead to unexpected issues.
Note: transferTokens and _transferTokens are very similar so they could be integrated; they have to be checked carefully when doing this
function transferTokens(.... , address _from, address _to ) { ... (bool success, ) = payable(address(msg.sender)).call{value: msg.value - _amount}(''); // uses msg.sender instead of _from // also uses - instead of sub
function _transferTokens( address _from, address _to,.... ) { (bool refundSuccess, ) = payable(_from).call{value: msg.value.sub(_amount)}('');
In function transferTokens() change msg.sender to _from